fix: update state values files handling to replace arrays instead of merging (#2537)

* feat: update state values handling to replace arrays instead of merging

Signed-off-by: Vojta Polak <vojta.polak@gmail.com>

* test: non-shallow copy of OverrideCLISetValues in DeepCopy + test

Signed-off-by: Vojta Polak <vojta.polak@gmail.com>

* fix: update error message for empty CalleePath in Load function

Signed-off-by: Vojta Polak <vojta.polak@gmail.com>

---------

Signed-off-by: Vojta Polak <vojta.polak@gmail.com>

pkg/app/app.go

@@ -1362,14 +1362,12 @@ func (a *App) visitStatesWithSelectorsAndRemoteSupportWithContext(fileOrDir stri
 	for _, v := range a.ValuesFiles {
 		envvals = append(envvals, v)
 	}
-
-	if len(a.Set) > 0 {
-		envvals = append(envvals, a.Set)
-	}
-
 	if len(envvals) > 0 {
 		opts.Environment.OverrideValues = envvals
-		opts.Environment.OverrideValuesAreCLI = true
+	}
+
+	if len(a.Set) > 0 {
+		opts.Environment.OverrideCLISetValues = []any{a.Set}
 	}
 
 	a.remote = remote.NewRemote(a.Logger, "", a.fs)

pkg/app/app_test.go

@@ -1283,6 +1283,67 @@ x:
 	}
 }
 
+// TestStateValuesFileArrayReplace verifies that arrays in --state-values-file
+// replace environment default arrays entirely rather than merging element-by-element.
+// Regression test for https://github.com/helmfile/helmfile/issues/2536
+func TestStateValuesFileArrayReplace(t *testing.T) {
+	files := map[string]string{
+		"/path/to/helmfile.yaml.gotmpl": `
+environments:
+  default:
+    values:
+    - env-defaults.yaml
+---
+releases:
+- name: {{ .Values.list | join "," }}
+  chart: stable/noop
+`,
+		// Environment default defines a list with two elements
+		"/path/to/env-defaults.yaml": `
+list:
+- first
+- second
+`,
+		// --state-values-file overrides the list with a single element
+		// This should REPLACE the list, not merge element-by-element.
+		"/path/to/overrides.yaml": `
+list:
+- second
+`,
+	}
+
+	actual := []state.ReleaseSpec{}
+	collectReleases := func(run *Run) (bool, []error) {
+		actual = append(actual, run.state.Releases...)
+		return false, []error{}
+	}
+
+	app := appWithFs(&App{
+		OverrideHelmBinary:              DefaultHelmBinary,
+		OverrideKubeContext:             "default",
+		DisableKubeVersionAutoDetection: true,
+		Logger:                          newAppTestLogger(),
+		Selectors:                       []string{},
+		Env:                             "default",
+		ValuesFiles:                     []string{"overrides.yaml"},
+		FileOrDir:                       "helmfile.yaml.gotmpl",
+	}, files)
+
+	expectNoCallsToHelm(app)
+
+	err := app.ForEachState(collectReleases, false, SetFilter(true))
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(actual) != 1 {
+		t.Fatalf("expected 1 release, got %d", len(actual))
+	}
+	// list should be ["second"] (replaced), not ["second","second"] (merged by position)
+	if actual[0].Name != "second" {
+		t.Errorf("expected release name %q (list replaced), got %q (list was merged)", "second", actual[0].Name)
+	}
+}
+
 func TestVisitDesiredStatesWithReleasesFiltered_ChartAtAbsPath(t *testing.T) {
 	files := map[string]string{
 		"/path/to/helmfile.yaml": `

pkg/app/desired_state_file_loader.go

@@ -50,30 +50,39 @@ type desiredStateLoader struct {
 func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, error) {
 	var overrodeEnv *environment.Environment
 
-	args := opts.Environment.OverrideValues
+	fileArgs := opts.Environment.OverrideValues
+	setArgs := opts.Environment.OverrideCLISetValues
 
-	if len(args) > 0 {
+	if len(fileArgs) > 0 || len(setArgs) > 0 {
 		if opts.CalleePath == "" {
-			return nil, fmt.Errorf("bug: opts.CalleePath was nil: f=%s, opts=%v", f, opts)
+			return nil, fmt.Errorf("bug: opts.CalleePath was empty: f=%s, opts=%v", f, opts)
 		}
 		storage := state.NewStorage(opts.CalleePath, ld.logger, ld.fs)
 		envld := state.NewEnvironmentValuesLoader(storage, ld.fs, ld.logger, ld.remote)
 		handler := state.MissingFileHandlerError
-		vals, err := envld.LoadEnvironmentValues(&handler, args, environment.New(ld.env), ld.env)
+
-		if err != nil {
+		overrodeEnv = &environment.Environment{
-			return nil, err
+			Name:         ld.env,
+			Values:       map[string]any{},
+			CLIOverrides: map[string]any{},
 		}
 
-		if opts.Environment.OverrideValuesAreCLI {
+		// --state-values-file: loaded into Values so arrays replace (not merge)
-			overrodeEnv = &environment.Environment{
+		if len(fileArgs) > 0 {
-				Name:         ld.env,
+			fileVals, err := envld.LoadEnvironmentValues(&handler, fileArgs, environment.New(ld.env), ld.env)
-				CLIOverrides: vals,
+			if err != nil {
+				return nil, err
 			}
-		} else {
+			overrodeEnv.Values = fileVals
-			overrodeEnv = &environment.Environment{
+		}
-				Name:   ld.env,
+
-				Values: vals,
+		// --state-values-set: loaded into CLIOverrides so arrays merge element-by-element
+		if len(setArgs) > 0 {
+			setVals, err := envld.LoadEnvironmentValues(&handler, setArgs, environment.New(ld.env), ld.env)
+			if err != nil {
+				return nil, err
 			}
+			overrodeEnv.CLIOverrides = setVals
 		}
 	}
 

pkg/app/load_opts.go

@@ -30,7 +30,17 @@ func (o LoadOpts) DeepCopy() LoadOpts {
 		panic(err)
 	}
 
-	new.Environment.OverrideValuesAreCLI = o.Environment.OverrideValuesAreCLI
+	if src := o.Environment.OverrideCLISetValues; src != nil {
+		b, err := yaml.Marshal(src)
+		if err != nil {
+			panic(err)
+		}
+		var dst []any
+		if err := yaml.Unmarshal(b, &dst); err != nil {
+			panic(err)
+		}
+		new.Environment.OverrideCLISetValues = dst
+	}
 
 	return new
 }

pkg/app/load_opts_test.go

@@ -21,16 +21,33 @@ func TestLoadOptsDeepCopy(t *testing.T) {
 	require.Equal(t, lOld, lNew, "DeepCopy should return a copy of the LoadOpts struct")
 }
 
-// TestLoadOptsDeepCopyPreservesOverrideValuesAreCLI verifies that DeepCopy
+// TestLoadOptsDeepCopyPreservesOverrideCLISetValues verifies that DeepCopy
-// preserves the OverrideValuesAreCLI flag which is tagged yaml:"-".
+// preserves the OverrideCLISetValues field which is tagged yaml:"-".
-func TestLoadOptsDeepCopyPreservesOverrideValuesAreCLI(t *testing.T) {
+func TestLoadOptsDeepCopyPreservesOverrideCLISetValues(t *testing.T) {
 	lOld := LoadOpts{
 		Selectors:  []string{"test"},
 		CalleePath: "test",
 	}
-	lOld.Environment.OverrideValuesAreCLI = true
+	lOld.Environment.OverrideCLISetValues = []any{map[string]any{"key": "value"}}
 
 	lNew := lOld.DeepCopy()
 
-	require.True(t, lNew.Environment.OverrideValuesAreCLI, "DeepCopy should preserve OverrideValuesAreCLI flag")
+	require.Equal(t, lOld.Environment.OverrideCLISetValues, lNew.Environment.OverrideCLISetValues, "DeepCopy should preserve OverrideCLISetValues field")
+}
+
+// TestLoadOptsDeepCopyOverrideCLISetValuesIsNotShallow verifies that mutating a
+// map nested inside OverrideCLISetValues on the copy does not affect the
+// original.
+func TestLoadOptsDeepCopyOverrideCLISetValuesIsNotShallow(t *testing.T) {
+	lOld := LoadOpts{}
+	lOld.Environment.OverrideCLISetValues = []any{map[string]any{"key": "original"}}
+
+	lNew := lOld.DeepCopy()
+
+	// Mutate the map inside the copy.
+	lNew.Environment.OverrideCLISetValues[0].(map[string]any)["key"] = "mutated"
+
+	// The original must be unaffected; this fails with a shallow copy.
+	require.Equal(t, "original", lOld.Environment.OverrideCLISetValues[0].(map[string]any)["key"],
+		"mutating the copy's OverrideCLISetValues map must not affect the original (aliasing bug)")
 }

pkg/state/state.go

@@ -156,7 +156,7 @@ type SubHelmfileSpec struct {
 // SubhelmfileEnvironmentSpec is the environment spec for a subhelmfile
 type SubhelmfileEnvironmentSpec struct {
 	OverrideValues       []any `yaml:"values,omitempty"`
-	OverrideValuesAreCLI bool  `yaml:"-"`
+	OverrideCLISetValues []any `yaml:"-"` // CLI --state-values-set values only, merged element-by-element
 }
 
 // HelmSpec to defines helmDefault values