-
-
Notifications
You must be signed in to change notification settings - Fork 174
fix(config): honor --config across internal reloads and multi-file merges #2875
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
472dc36
571f0d2
e98a7c5
13d7399
dda284e
f2d34be
224a0b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,22 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "bytes" | ||
| stdio "io" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| ckerrors "github.com/cockroachdb/errors" | ||
| "github.com/spf13/cobra" | ||
| "github.com/spf13/viper" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| errUtils "github.com/cloudposse/atmos/errors" | ||
| "github.com/cloudposse/atmos/pkg/data" | ||
| iolib "github.com/cloudposse/atmos/pkg/io" | ||
| "github.com/cloudposse/atmos/pkg/schema" | ||
| atmosyaml "github.com/cloudposse/atmos/pkg/yaml" | ||
| ) | ||
|
|
@@ -130,6 +137,29 @@ func TestResolveConfigFile_Error(t *testing.T) { | |
| require.ErrorIs(t, err, errUtils.ErrInvalidArgumentError) | ||
| } | ||
|
|
||
| // TestResolveConfigFile_MultipleConfigFilesAmbiguous guards against a bug found during a | ||
| // field-test pass on cloudposse/atmos#2867/#2868: resolveConfigFile silently used only the | ||
| // FIRST --config file (cfgFiles[0]) when multiple were given, so `config set --config a,b | ||
| // logs.level X` reported success editing a.yaml while the actual effective value (what `config | ||
| // get` reports, and what every other atmos command uses) stayed unchanged whenever b.yaml also | ||
| // set that key -- a false success, not just a stale-value bug. | ||
| func TestResolveConfigFile_MultipleConfigFilesAmbiguous(t *testing.T) { | ||
| dir := t.TempDir() | ||
| fileA := filepath.Join(dir, "a.yaml") | ||
| fileB := filepath.Join(dir, "b.yaml") | ||
| require.NoError(t, os.WriteFile(fileA, []byte("settings:\n enabled: true\n"), 0o644)) | ||
| require.NoError(t, os.WriteFile(fileB, []byte("settings:\n enabled: false\n"), 0o644)) | ||
|
|
||
| cmd := &cobra.Command{} | ||
| cmd.Flags().StringSlice("config", []string{fileA, fileB}, "") | ||
|
|
||
| _, err := resolveConfigFile(cmd) | ||
| require.ErrorIs(t, err, errUtils.ErrInvalidArgumentError) | ||
| details := strings.Join(ckerrors.GetAllDetails(err), "\n") | ||
| assert.Contains(t, details, "a.yaml") | ||
| assert.Contains(t, details, "b.yaml") | ||
| } | ||
|
|
||
| func TestConfigGetCommand_MissingValue(t *testing.T) { | ||
| dir := t.TempDir() | ||
| file := filepath.Join(dir, "atmos.yaml") | ||
|
|
@@ -146,6 +176,86 @@ func TestConfigGetCommand_MissingValue(t *testing.T) { | |
| require.ErrorIs(t, err, atmosyaml.ErrYAMLPathNotFound) | ||
| } | ||
|
|
||
| // configGetTestStreams is a minimal io.Streams implementation for capturing data output, | ||
| // mirroring configSchemaTestStreams in schema_test.go. | ||
| type configGetTestStreams struct { | ||
| stdin stdio.Reader | ||
| stdout *bytes.Buffer | ||
| stderr *bytes.Buffer | ||
| } | ||
|
|
||
| func (ts *configGetTestStreams) Input() stdio.Reader { return ts.stdin } | ||
| func (ts *configGetTestStreams) Output() stdio.Writer { return ts.stdout } | ||
| func (ts *configGetTestStreams) Error() stdio.Writer { return ts.stderr } | ||
| func (ts *configGetTestStreams) RawOutput() stdio.Writer { return ts.stdout } | ||
| func (ts *configGetTestStreams) RawError() stdio.Writer { return ts.stderr } | ||
|
|
||
| // TestConfigGetCommand_ReportsEffectiveMergedValue reproduces the "stale value" half of | ||
| // cloudposse/atmos#2867: `atmos config get` used to read only the FIRST --config file | ||
| // directly off disk (resolveConfigFile picked cfgFiles[0]), so a second --config file's | ||
| // override was invisible to `get` even though the rest of atmos (stack discovery, etc.) | ||
| // correctly used the merged value. `get` must report the same effective value everything | ||
| // else uses. | ||
| func TestConfigGetCommand_ReportsEffectiveMergedValue(t *testing.T) { | ||
| dir := t.TempDir() | ||
| mainFile := filepath.Join(dir, "main.yaml") | ||
| fragmentFile := filepath.Join(dir, "fragment.yaml") | ||
|
|
||
| require.NoError(t, os.WriteFile(mainFile, []byte(` | ||
| base_path: "." | ||
| stacks: | ||
| base_path: "stacks" | ||
| included_paths: | ||
| - "deploy/**/*" | ||
| `), 0o644)) | ||
| require.NoError(t, os.WriteFile(fragmentFile, []byte(` | ||
| stacks: | ||
| included_paths: | ||
| - "deploy/**/*" | ||
| - "other/**/*" | ||
| `), 0o644)) | ||
|
|
||
| streams := &configGetTestStreams{stdin: &bytes.Buffer{}, stdout: &bytes.Buffer{}, stderr: &bytes.Buffer{}} | ||
| ioCtx, err := iolib.NewContext(iolib.WithStreams(streams)) | ||
| require.NoError(t, err) | ||
| data.InitWriter(ioCtx) | ||
| t.Cleanup(data.Reset) | ||
|
|
||
| viper.Reset() | ||
| t.Cleanup(viper.Reset) | ||
|
|
||
| origArgs := os.Args | ||
| t.Cleanup(func() { os.Args = origArgs }) | ||
| os.Args = []string{"atmos", "--config", mainFile + "," + fragmentFile, "config", "get", "stacks.included_paths"} | ||
|
|
||
| require.NoError(t, configGetCmd.RunE(configGetCmd, []string{"stacks.included_paths"})) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| output := streams.stdout.String() | ||
| assert.True(t, strings.Contains(output, "deploy/**/*"), "output should contain the first file's value: %s", output) | ||
| assert.True(t, strings.Contains(output, "other/**/*"), | ||
| "output must reflect the SECOND --config file's override, not just the first file's stale value: %s", output) | ||
| } | ||
|
|
||
| // TestConfigGetCommand_InitCliConfigError proves configGetCmd.RunE surfaces a genuine | ||
| // InitCliConfig failure (a malformed --config file here) instead of panicking or masking it, | ||
| // since `get` now reloads the full effective config on every invocation rather than reading a | ||
| // single already-validated file (cloudposse/atmos#2867/#2868). | ||
| func TestConfigGetCommand_InitCliConfigError(t *testing.T) { | ||
| dir := t.TempDir() | ||
| badFile := filepath.Join(dir, "bad.yaml") | ||
| require.NoError(t, os.WriteFile(badFile, []byte("settings:\n enabled: [true\n"), 0o644)) // unterminated flow sequence | ||
|
|
||
| viper.Reset() | ||
| t.Cleanup(viper.Reset) | ||
|
|
||
| origArgs := os.Args | ||
| t.Cleanup(func() { os.Args = origArgs }) | ||
| os.Args = []string{"atmos", "--config", badFile, "config", "get", "settings.enabled"} | ||
|
|
||
| err := configGetCmd.RunE(configGetCmd, []string{"settings.enabled"}) | ||
| require.Error(t, err) | ||
| } | ||
|
Comment on lines
+243
to
+257
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Isolate command state with Create As per coding guidelines: “Always use 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| func TestConfigSetCommand_TypeVariants(t *testing.T) { | ||
| dir := t.TempDir() | ||
| file := filepath.Join(dir, "atmos.yaml") | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.