Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Porter config file doesn't always bind to the command flag #2735

Closed
carolynvs opened this issue Apr 19, 2023 · 0 comments · Fixed by #2738
Closed

Porter config file doesn't always bind to the command flag #2735

carolynvs opened this issue Apr 19, 2023 · 0 comments · Fixed by #2738
Assignees
Labels
0 - 🚨🍩 Emergency donuts come first bug Oops, sorry!

Comments

@carolynvs
Copy link
Member

Describe the bug

When running a porter command that has a flag is defined, and that setting is not globally defined on config.DataStore, configuring the flag in your porter config file doesn't work. Only environment variables and directly setting the flag works.

Context

  • DataStore is reserved for settings that are not exposed on command flags, and should only be defined for settings such as the current secrets plugin.
  • The root cause is that we are binding viper config values to the cobra flags too early, before the config file has been loaded.

To Reproduce

Let's pick a flag that is defined on a command that doesn't have a global setting, such as porter build --no-lint.

  1. Create a bundle
  2. Edit the install action with the snippet below which normally triggers a lint error
     install:
       - exec:
           description: quotes are for suckers
           command: bash
           flags:
             c: echo "Hello World"
  3. Run porter build --no-lint=true and note that the build is stopped with a lint error.
  4. Run porter build --no-lint=false and note that the build is allowed to execute.
  5. Now edit your ~/.porter/config.yaml and add a line with no-lint: true.
  6. Run porter build without specifying the --no-lint flag. Note that the linter is still executed and the build is stopped with a lint error.

Expected behavior

The configuration file should automatically set --no-lint to true and the build should be allowed to proceed.

Porter Command and Output

$ porter build
warning(exec-100) - Best Practice: Avoid Embedded Bash
install: 1st step in the exec mixin (quotes are for suckers)
See https://getporter.org/best-practices/exec-mixin/#use-scripts for more information
---
error(exec-101) - bash -c argument missing wrapping quotes
install: 1st step in the exec mixin (quotes are for suckers)
The bash -c flag argument must be wrapped in quotes, for example
exec:
  description: Say Hello
  command: bash
  flags:
    c: '"echo Hello World"'

See https://getporter.org/best-practices/exec-mixin/#quoting-escaping-bash-and-yaml for more information
---

lint errors were detected. Rerun with --no-lint ignore the errors

Version

1.0.13

@carolynvs carolynvs added the bug Oops, sorry! label Apr 19, 2023
@carolynvs carolynvs self-assigned this Apr 19, 2023
@carolynvs carolynvs added the 0 - 🚨🍩 Emergency donuts come first label Apr 19, 2023
carolynvs added a commit to carolynvs/porter that referenced this issue Apr 20, 2023
When we were binding viper values to the cobra flags, it was occuring before the config file was loaded, so if you configured a flag in the config file, it wasn't being applied to the flag correctly. It worked fine for environment variables (such as PORTER_NO_LINT) or using the flag directly (such as --no-lint).

I have split our viper configuration into a two step process:

1. Configure the viper instance, for example binding custom open telemetry environment variables.
2. Bind viper to the cobra flags, AFTER the config file has been loaded.

This ensures that viper has all relevant configuration loaded before binding its value back to the original command flags (i.e. our variables in code).

Fixes getporter#2735

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs added a commit to carolynvs/porter that referenced this issue Apr 20, 2023
When we were binding viper values to the cobra flags, it was occuring before the config file was loaded, so if you configured a flag in the config file, it wasn't persisting the correct value to the variable bound to the flag.

When the flag was not bound to the global porter Config.Data, such as porter build --no-lint, the configuration file values were not applied. This is the regression describe in getporter#2735 which was caused by apply cobra values to viper too soon (before the config file was loaded).

I have split our viper configuration into a three step process:

1. Configure the viper instance, for example binding custom open telemetry environment variables.
2. Bind viper to the cobra flags, AFTER the config file has been loaded.
3. Apply the consolidated configuration in viper (which now contains both cobra flags, env vars and the config file) back to the global config in Config.Data

This ensures that viper has all relevant configuration loaded before binding its value back to the original command flags (i.e. our variables in code).

Fixes getporter#2735

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs added a commit to carolynvs/porter that referenced this issue Apr 24, 2023
When we were binding viper values to the cobra flags, it was occuring before the config file was loaded, so if you configured a flag in the config file, it wasn't persisting the correct value to the variable bound to the flag.

When the flag was not bound to the global porter Config.Data, such as porter build --no-lint, the configuration file values were not applied. This is the regression describe in getporter#2735 which was caused by apply cobra values to viper too soon (before the config file was loaded).

I have split our viper configuration into a three step process:

1. Configure the viper instance, for example binding custom open telemetry environment variables.
2. Bind viper to the cobra flags, AFTER the config file has been loaded.
3. Apply the consolidated configuration in viper (which now contains both cobra flags, env vars and the config file) back to the global config in Config.Data

This ensures that viper has all relevant configuration loaded before binding its value back to the original command flags (i.e. our variables in code).

Fixes getporter#2735

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
bdegeeter pushed a commit to bdegeeter/porter that referenced this issue May 11, 2023
When we were binding viper values to the cobra flags, it was occuring before the config file was loaded, so if you configured a flag in the config file, it wasn't persisting the correct value to the variable bound to the flag.

When the flag was not bound to the global porter Config.Data, such as porter build --no-lint, the configuration file values were not applied. This is the regression describe in getporter#2735 which was caused by apply cobra values to viper too soon (before the config file was loaded).

I have split our viper configuration into a three step process:

1. Configure the viper instance, for example binding custom open telemetry environment variables.
2. Bind viper to the cobra flags, AFTER the config file has been loaded.
3. Apply the consolidated configuration in viper (which now contains both cobra flags, env vars and the config file) back to the global config in Config.Data

This ensures that viper has all relevant configuration loaded before binding its value back to the original command flags (i.e. our variables in code).

Fixes getporter#2735

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - 🚨🍩 Emergency donuts come first bug Oops, sorry!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant