diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 7ec1999c..bd42a3b3 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -4618,3 +4618,47 @@ func TestRenderYamlEnvVar(t *testing.T) { }) } } + +// TestHelmBinaryPreservedInMultiDocumentYAML tests that helmBinary is preserved +// when processing multi-document YAML files at the loadDesiredStateFromYaml level. +// This is a regression test for issue #2319. +func TestHelmBinaryPreservedInMultiDocumentYAML(t *testing.T) { + statePath := "/path/to/helmfile.yaml" + stateContent := `helmBinary: /usr/bin/werf + +helmDefaults: + createNamespace: true + +--- + +releases: + - name: myapp + chart: stable/nginx +` + testFs := testhelper.NewTestFs(map[string]string{ + statePath: stateContent, + }) + + app := &App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "", + DisableKubeVersionAutoDetection: true, + Logger: newAppTestLogger(), + Namespace: "", + Env: "default", + FileOrDir: statePath, + } + app = injectFs(app, testFs) + app.remote = remote.NewRemote(app.Logger, testFs.Cwd, app.fs) + + expectNoCallsToHelm(app) + + st, err := app.loadDesiredStateFromYaml(statePath, LoadOpts{}) + require.NoError(t, err, "unexpected error loading helmfile") + require.NotNil(t, st, "expected state to be loaded") + + // The key assertion: helmBinary from document 1 should be preserved + // even though document 2 has no helmBinary setting + assert.Equal(t, "/usr/bin/werf", st.DefaultHelmBinary, + "helmBinary from first document should be preserved, not reset to default 'helm'") +} diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index 40452bec..deb833f0 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -185,7 +185,8 @@ func (a *desiredStateLoader) rawLoad(yaml []byte, baseDir, file string, evaluate return nil, err } - st, err = a.underlying().ParseAndLoad(yaml, baseDir, file, a.env, false, evaluateBases, merged, nil) + // applyDefaults is always false here - defaults are applied after all parts are merged + st, err = a.underlying().ParseAndLoad(yaml, baseDir, file, a.env, false, evaluateBases, false, merged, nil) if err != nil { return nil, err } @@ -297,6 +298,14 @@ func (ld *desiredStateLoader) load(env, overrodeEnv *environment.Environment, ba } } + // After all parts are merged, apply defaults and overrides to ensure + // that values from earlier parts (like helmBinary) are preserved correctly + // in the merged state. + // See https://github.com/helmfile/helmfile/issues/2319 + if evaluateBases { + ld.underlying().ApplyDefaultsAndOverrides(finalState) + } + // If environments are not defined in the helmfile at all although the env is specified, // it's a missing env situation. Let's fail. if len(finalState.Environments) == 0 && evaluateBases && !hasEnv && env.Name != state.DefaultEnv { diff --git a/pkg/state/create.go b/pkg/state/create.go index beb48ae3..9058e9a7 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -137,6 +137,12 @@ func (c *StateCreator) Parse(content []byte, baseDir, file string) (*HelmState, return &state, nil } +// ApplyDefaultsAndOverrides applies default binary paths and command-line overrides. +// This is an exported version of applyDefaultsAndOverrides for use by the app package. +func (c *StateCreator) ApplyDefaultsAndOverrides(state *HelmState) { + c.applyDefaultsAndOverrides(state) +} + // applyDefaultsAndOverrides applies default binary paths and command-line overrides func (c *StateCreator) applyDefaultsAndOverrides(state *HelmState) { if c.overrideHelmBinary != "" && c.overrideHelmBinary != DefaultHelmBinary { @@ -179,7 +185,8 @@ func (c *StateCreator) LoadEnvValues(target *HelmState, env string, failOnMissin // Parses YAML into HelmState, while loading environment values files relative to the `baseDir` // evaluateBases=true means that this is NOT a base helmfile -func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envName string, failOnMissingEnv, evaluateBases bool, envValues, overrode *environment.Environment) (*HelmState, error) { +// applyDefaults=true means that applyDefaultsAndOverrides should be called +func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envName string, failOnMissingEnv, evaluateBases, applyDefaults bool, envValues, overrode *environment.Environment) (*HelmState, error) { state, err := c.Parse(content, baseDir, file) if err != nil { return nil, err @@ -198,7 +205,9 @@ func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envNam // Apply default binaries and command-line overrides only for the main helmfile // after loading and merging all bases. This ensures that values from bases are // properly respected and that later bases/documents can override earlier ones. - c.applyDefaultsAndOverrides(state) + if applyDefaults { + c.applyDefaultsAndOverrides(state) + } } state, err = c.LoadEnvValues(state, envName, failOnMissingEnv, envValues, overrode) diff --git a/pkg/state/create_test.go b/pkg/state/create_test.go index 0fff0173..aa6b704e 100644 --- a/pkg/state/create_test.go +++ b/pkg/state/create_test.go @@ -6,8 +6,11 @@ import ( "reflect" "testing" + "dario.cat/mergo" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" + "go.uber.org/zap/zaptest" "github.com/helmfile/helmfile/pkg/environment" "github.com/helmfile/helmfile/pkg/filesystem" @@ -21,7 +24,7 @@ func createFromYaml(content []byte, file string, env string, logger *zap.Sugared fs: filesystem.DefaultFileSystem(), Strict: true, } - return c.ParseAndLoad(content, filepath.Dir(file), file, env, false, true, nil, nil) + return c.ParseAndLoad(content, filepath.Dir(file), file, env, false, true, true, nil, nil) } func TestReadFromYaml(t *testing.T) { @@ -86,7 +89,7 @@ func (testEnv stateTestEnv) MustLoadStateWithEnableLiveOutput(t *testing.T, file r := remote.NewRemote(logger, testFs.Cwd, testFs.ToFileSystem()) state, err := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", "", r, enableLiveOutput, ""). - ParseAndLoad([]byte(yamlContent), filepath.Dir(file), file, envName, true, true, nil, nil) + ParseAndLoad([]byte(yamlContent), filepath.Dir(file), file, envName, true, true, true, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -156,7 +159,7 @@ releaseNamespace: mynamespace Name: "production", } state, err := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", "", r, false, ""). - ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, &env, nil) + ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, true, &env, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -243,7 +246,7 @@ overrideNamespace: myns r := remote.NewRemote(logger, testFs.Cwd, testFs.ToFileSystem()) state, err := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", "", r, false, ""). - ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, nil, nil) + ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, true, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -507,7 +510,7 @@ releaseContext: r := remote.NewRemote(logger, testFs.Cwd, testFs.ToFileSystem()) state, err := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", "", r, false, ""). - ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, nil, nil) + ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, true, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -703,7 +706,11 @@ bases: if !ok { return nil, fmt.Errorf("file not found: %s", path) } - return creator.ParseAndLoad([]byte(content), filepath.Dir(path), path, DefaultEnv, true, evaluateBases, inheritedEnv, overrodeEnv) + // When loading base files, we don't apply defaults - they should only + // be applied to the main file after all parts/bases are merged. + // So applyDefaults = evaluateBases (true for main, false for bases). + applyDefaults := evaluateBases + return creator.ParseAndLoad([]byte(content), filepath.Dir(path), path, DefaultEnv, true, evaluateBases, applyDefaults, inheritedEnv, overrodeEnv) } yamlContent, ok := tt.files[tt.mainFile] @@ -711,7 +718,7 @@ bases: t.Fatalf("no file named %q registered", tt.mainFile) } - state, err := creator.ParseAndLoad([]byte(yamlContent), filepath.Dir(tt.mainFile), tt.mainFile, DefaultEnv, true, true, nil, nil) + state, err := creator.ParseAndLoad([]byte(yamlContent), filepath.Dir(tt.mainFile), tt.mainFile, DefaultEnv, true, true, true, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -729,6 +736,48 @@ bases: } } +// TestHelmBinaryInMultiDocumentYAML tests that helmBinary is preserved when +// processing multi-document YAML files (files with --- separators). +// This is a regression test for issue #2319. +func TestHelmBinaryInMultiDocumentYAML(t *testing.T) { + logger := zaptest.NewLogger(t).Sugar() + testFs := testhelper.NewTestFs(map[string]string{}) + testFs.Cwd = "/" + + r := remote.NewRemote(logger, testFs.Cwd, testFs.ToFileSystem()) + creator := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", "", r, false, "") + + // Simulate a multi-document YAML where the first document sets helmBinary + // and the second document has no helmBinary + doc1 := `helmBinary: /custom/helm +helmDefaults: + timeout: 300 +` + doc2 := `releases: + - name: myapp + chart: stable/nginx +` + + // Simulate what load() in desired_state_file_loader.go does: + // 1. Process each part with applyDefaults=false + state1, err := creator.ParseAndLoad([]byte(doc1), "/", "/helmfile.yaml", DefaultEnv, true, true, false, nil, nil) + require.NoError(t, err) + + state2, err := creator.ParseAndLoad([]byte(doc2), "/", "/helmfile.yaml", DefaultEnv, true, true, false, nil, nil) + require.NoError(t, err) + + // 2. Merge parts (simulating what mergo.Merge does with mergo.WithOverride) + // Since state2 has empty DefaultHelmBinary, mergo.WithOverride should preserve state1's value + require.NoError(t, mergo.Merge(state1, state2, mergo.WithAppendSlice, mergo.WithOverride)) + + // 3. Apply defaults after merge + creator.ApplyDefaultsAndOverrides(state1) + + // Verify that helmBinary from first document is preserved + assert.Equal(t, "/custom/helm", state1.DefaultHelmBinary, + "helmBinary from first document should be preserved after merge and applyDefaults") +} + // TestEnvironmentMergingWithBases tests that environment values from multiple bases // are properly merged rather than replaced. This is a regression test for issue #2273. func TestEnvironmentMergingWithBases(t *testing.T) { @@ -858,7 +907,11 @@ releases: if !ok { return nil, fmt.Errorf("file not found: %s", path) } - return creator.ParseAndLoad([]byte(content), filepath.Dir(path), path, tt.environment, true, evaluateBases, inheritedEnv, overrodeEnv) + // When loading base files, we don't apply defaults - they should only + // be applied to the main file after all parts/bases are merged. + // So applyDefaults = evaluateBases (true for main, false for bases). + applyDefaults := evaluateBases + return creator.ParseAndLoad([]byte(content), filepath.Dir(path), path, tt.environment, true, evaluateBases, applyDefaults, inheritedEnv, overrodeEnv) } yamlContent, ok := tt.files[tt.mainFile] @@ -866,7 +919,7 @@ releases: t.Fatalf("no file named %q registered", tt.mainFile) } - state, err := creator.ParseAndLoad([]byte(yamlContent), filepath.Dir(tt.mainFile), tt.mainFile, tt.environment, true, true, nil, nil) + state, err := creator.ParseAndLoad([]byte(yamlContent), filepath.Dir(tt.mainFile), tt.mainFile, tt.environment, true, true, true, nil, nil) if tt.expectedError { require.Error(t, err, "expected an error but got none") return