From bca4e96bf0329b18fec656cada30ef8cd40538fb Mon Sep 17 00:00:00 2001 From: Vojta Polak Date: Fri, 10 Apr 2026 14:47:20 +0200 Subject: [PATCH] feat: update state values handling to replace arrays instead of merging Signed-off-by: Vojta Polak --- pkg/app/app.go | 10 ++--- pkg/app/app_test.go | 61 ++++++++++++++++++++++++++++ pkg/app/desired_state_file_loader.go | 35 ++++++++++------ pkg/app/load_opts.go | 2 +- pkg/app/load_opts_test.go | 10 ++--- pkg/state/state.go | 2 +- 6 files changed, 94 insertions(+), 26 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index 9b0da47c..7cbee422 100644 --- a/pkg/app/app.go +++ b/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) diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 21ec2d4e..73697915 100644 --- a/pkg/app/app_test.go +++ b/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": ` diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index 03064c47..0da8f0d9 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/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) } 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 { - return nil, err + + overrodeEnv = &environment.Environment{ + Name: ld.env, + Values: map[string]any{}, + CLIOverrides: map[string]any{}, } - if opts.Environment.OverrideValuesAreCLI { - overrodeEnv = &environment.Environment{ - Name: ld.env, - CLIOverrides: vals, + // --state-values-file: loaded into Values so arrays replace (not merge) + if len(fileArgs) > 0 { + fileVals, err := envld.LoadEnvironmentValues(&handler, fileArgs, environment.New(ld.env), ld.env) + if err != nil { + return nil, err } - } else { - overrodeEnv = &environment.Environment{ - Name: ld.env, - Values: vals, + overrodeEnv.Values = fileVals + } + + // --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 } } diff --git a/pkg/app/load_opts.go b/pkg/app/load_opts.go index fc7d8bfb..d108acbc 100644 --- a/pkg/app/load_opts.go +++ b/pkg/app/load_opts.go @@ -30,7 +30,7 @@ func (o LoadOpts) DeepCopy() LoadOpts { panic(err) } - new.Environment.OverrideValuesAreCLI = o.Environment.OverrideValuesAreCLI + new.Environment.OverrideCLISetValues = o.Environment.OverrideCLISetValues return new } diff --git a/pkg/app/load_opts_test.go b/pkg/app/load_opts_test.go index 7db6f1fb..c7dd4f9d 100644 --- a/pkg/app/load_opts_test.go +++ b/pkg/app/load_opts_test.go @@ -21,16 +21,16 @@ func TestLoadOptsDeepCopy(t *testing.T) { require.Equal(t, lOld, lNew, "DeepCopy should return a copy of the LoadOpts struct") } -// TestLoadOptsDeepCopyPreservesOverrideValuesAreCLI verifies that DeepCopy -// preserves the OverrideValuesAreCLI flag which is tagged yaml:"-". -func TestLoadOptsDeepCopyPreservesOverrideValuesAreCLI(t *testing.T) { +// TestLoadOptsDeepCopyPreservesOverrideCLISetValues verifies that DeepCopy +// preserves the OverrideCLISetValues field which is tagged yaml:"-". +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") } diff --git a/pkg/state/state.go b/pkg/state/state.go index 9fe36558..efa248c2 100644 --- a/pkg/state/state.go +++ b/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