fix: helmBinary setting ignored in multi-document YAML files (#2414)

* fix: helmBinary setting ignored in multi-document YAML files The helmBinary setting in helmfile.yaml was being ignored when using multi-document YAML files (files with --- separators). Root Cause: When processing multi-document YAML files, the load() function splits the file into parts and processes each part separately. Each part was calling applyDefaultsAndOverrides() which would set an empty helmBinary to the default 'helm'. When merging parts, the default value from a later part would override the correct value from an earlier part. Fix: - Added a new applyDefaults parameter to ParseAndLoad() to control when defaults are applied - Modified rawLoad() to pass applyDefaults=false when processing individual parts - Added a call to ApplyDefaultsAndOverrides() after all parts are merged to apply defaults once on the final merged state - Exported ApplyDefaultsAndOverrides() method for use by the app package Fixes: #2319 Signed-off-by: yxxhero <aiopsclub@163.com> * fix: update comment per PR review Change 're-apply' to 'apply' since defaults are never applied during part processing (applyDefaults=false is passed), so this is the first and only time defaults are applied to the merged state. Signed-off-by: yxxhero <aiopsclub@163.com> * fix: clarify applyDefaults logic in test LoadFile callbacks Add explicit applyDefaults variable with comment explaining why it equals evaluateBases: base files shouldn't apply defaults, only the main file should after all parts/bases are merged. Signed-off-by: yxxhero <aiopsclub@163.com> * fix: address PR review comments - Remove applyDefaults parameter from rawLoad() since it's always false - Add regression test for multi-document YAML with helmBinary (issue #2319) Signed-off-by: yxxhero <aiopsclub@163.com> * test: add integration test for helmBinary in multi-document YAML Add TestHelmBinaryPreservedInMultiDocumentYAML that exercises the full loadDesiredStateFromYaml path to ensure helmBinary from the first document is preserved when merging multi-document YAML files. This is a regression test at the load() orchestration level for issue #2319. Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: yxxhero <aiopsclub@163.com>
2026-02-20 22:12:38 +08:00 · 2026-02-20 22:12:38 +08:00 · ca8fc293e9
parent cd918b79d1
commit ca8fc293e9
4 changed files with 127 additions and 12 deletions
--- 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'")
+}
--- 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 {
--- 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)
--- 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