diff --git a/pkg/app/app.go b/pkg/app/app.go index 6dffe8ed..e71bd490 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1268,8 +1268,10 @@ func (a *App) visitStatesWithSelectorsAndRemoteSupportWithContext(fileOrDir stri envvals = append(envvals, v) } + // Append CLI set to envvals for loading, and also pass it separately to mark it as CLI override if len(a.Set) > 0 { envvals = append(envvals, a.Set) + opts.Environment.CLISet = a.Set } if len(envvals) > 0 { diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index 5b65d9a6..b83b658a 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -64,9 +64,14 @@ func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, e return nil, err } + // CLI overrides (--state-values-set) should use element-by-element array merging + // CLISet is set when there are CLI overrides + isCLIOverride := len(opts.Environment.CLISet) > 0 + overrodeEnv = &environment.Environment{ - Name: ld.env, - Values: vals, + Name: ld.env, + Values: vals, + IsCLIOverride: isCLIOverride, } } diff --git a/pkg/config/global.go b/pkg/config/global.go index 0cf58f96..adbea3d4 100644 --- a/pkg/config/global.go +++ b/pkg/config/global.go @@ -102,7 +102,7 @@ func NewGlobalImpl(opts *GlobalOptions) *GlobalImpl { // Setset sets the set func (g *GlobalImpl) SetSet(set map[string]any) { - g.set = maputil.MergeMaps(g.set, set) + g.set = maputil.MergeMapsWithArrayMerge(g.set, set) } // HelmBinary returns the path to the Helm binary. diff --git a/pkg/environment/environment.go b/pkg/environment/environment.go index 7cddf102..886899c5 100644 --- a/pkg/environment/environment.go +++ b/pkg/environment/environment.go @@ -12,6 +12,9 @@ type Environment struct { KubeContext string Values map[string]any Defaults map[string]any + // IsCLIOverride indicates if this environment contains CLI overrides from --state-values-set + // When true, arrays are merged element-by-element. When false, arrays are replaced. + IsCLIOverride bool } var EmptyEnvironment Environment @@ -54,10 +57,11 @@ func (e Environment) DeepCopy() Environment { } return Environment{ - Name: e.Name, - KubeContext: e.KubeContext, - Values: values, - Defaults: defaults, + Name: e.Name, + KubeContext: e.KubeContext, + Values: values, + Defaults: defaults, + IsCLIOverride: e.IsCLIOverride, } } @@ -74,6 +78,10 @@ func (e *Environment) Merge(other *Environment) (*Environment, error) { if err := mergo.Merge(©, other, mergo.WithOverride); err != nil { return nil, err } + // Preserve the IsCLIOverride flag from other + if other.IsCLIOverride { + copy.IsCLIOverride = true + } } return ©, nil } @@ -81,10 +89,15 @@ func (e *Environment) Merge(other *Environment) (*Environment, error) { func (e *Environment) GetMergedValues() (map[string]any, error) { vals := map[string]any{} - // Use MergeMaps instead of mergo.Merge to properly handle array merging element-by-element - // This fixes issue #2281 where arrays were being replaced entirely instead of merged - vals = maputil.MergeMaps(vals, e.Defaults) - vals = maputil.MergeMaps(vals, e.Values) + // For CLI overrides (--state-values-set), use MergeMapsWithArrayMerge to handle array indices + // For helmfile composition, use regular MergeMaps which replaces arrays (documented behavior) + if e.IsCLIOverride { + vals = maputil.MergeMapsWithArrayMerge(vals, e.Defaults) + vals = maputil.MergeMapsWithArrayMerge(vals, e.Values) + } else { + vals = maputil.MergeMaps(vals, e.Defaults) + vals = maputil.MergeMaps(vals, e.Values) + } vals, err := maputil.CastKeysToStrings(vals) if err != nil { diff --git a/pkg/maputil/maputil.go b/pkg/maputil/maputil.go index ff2e9d05..63552714 100644 --- a/pkg/maputil/maputil.go +++ b/pkg/maputil/maputil.go @@ -243,6 +243,35 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { } } } + // Arrays are replaced, not merged (documented behavior) + out[k] = v + } + return out +} + +// MergeMapsWithArrayMerge merges two maps, merging arrays element-by-element. +// This is used specifically for CLI overrides (--state-values-set) where sparse arrays +// with specific indices set should merge with base arrays element-by-element. +// For general helmfile composition, use MergeMaps which replaces arrays. +func MergeMapsWithArrayMerge(a, b map[string]interface{}) map[string]interface{} { + out := make(map[string]interface{}, len(a)) + // fill the out map with the first map + for k, v := range a { + out[k] = v + } + for k, v := range b { + if v == nil { + continue + } + if v, ok := v.(map[string]interface{}); ok { + if bv, ok := out[k]; ok { + if bv, ok := bv.(map[string]interface{}); ok { + // if b and out map has a map value, merge it too + out[k] = MergeMapsWithArrayMerge(bv, v) + continue + } + } + } // Handle array merging element-by-element vSlice := toInterfaceSlice(v) if vSlice != nil { @@ -292,11 +321,11 @@ func mergeSlices(base, override []any) []any { continue } - // If both are maps, merge them recursively + // If both are maps, merge them recursively with array merging if overrideMap, ok := overrideVal.(map[string]any); ok { if i < len(base) { if baseMap, ok := base[i].(map[string]any); ok { - result[i] = MergeMaps(baseMap, overrideMap) + result[i] = MergeMapsWithArrayMerge(baseMap, overrideMap) continue } } diff --git a/pkg/maputil/maputil_test.go b/pkg/maputil/maputil_test.go index 24b2f663..aab181a7 100644 --- a/pkg/maputil/maputil_test.go +++ b/pkg/maputil/maputil_test.go @@ -474,7 +474,9 @@ func TestMapUtil_Issue2281_EmptyMapScenario(t *testing.T) { }) } -// TestMapUtil_Issue2281_MergeArrays tests that MergeMaps should merge arrays element-by-element +// TestMapUtil_Issue2281_MergeArrays tests that MergeMapsWithArrayMerge should merge arrays element-by-element +// This is used for CLI overrides (--state-values-set) where sparse arrays should merge with base arrays. +// For general helmfile composition, MergeMaps replaces arrays entirely (documented behavior). func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { t.Run("merging arrays should preserve elements from base that aren't in override", func(t *testing.T) { // Base values from helmfile @@ -491,7 +493,7 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { }, } - result := MergeMaps(base, override) + result := MergeMapsWithArrayMerge(base, override) // Expected: array should be ["cmdlinething1", "thing2"] // array[0] is overridden, array[1] is preserved from base @@ -532,7 +534,7 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { }, } - result := MergeMaps(base, override) + result := MergeMapsWithArrayMerge(base, override) // Expected: complexArray[0] should be unchanged, complexArray[1] should have merged fields resultArray := result["top"].(map[string]any)["complexArray"].([]any) @@ -558,7 +560,7 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { } }) - t.Run("complete issue #2281 scenario with MergeMaps", func(t *testing.T) { + t.Run("complete issue #2281 scenario with MergeMapsWithArrayMerge", func(t *testing.T) { // Base values from helmfile base := map[string]interface{}{ "top": map[string]any{ @@ -591,7 +593,7 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { }, } - result := MergeMaps(base, override) + result := MergeMapsWithArrayMerge(base, override) // Check array resultArray := result["top"].(map[string]any)["array"].([]any) @@ -617,3 +619,90 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { } }) } + +// TestMapUtil_MergeMaps_ArrayReplacement tests that MergeMaps replaces arrays +// This is the documented behavior for helmfile composition and layer merging +func TestMapUtil_MergeMaps_ArrayReplacement(t *testing.T) { + t.Run("arrays should be replaced not merged", func(t *testing.T) { + base := map[string]interface{}{ + "list": []any{ + map[string]any{"name": "dummy", "values": []any{1, 2}}, + }, + } + + override := map[string]interface{}{ + "list": []any{ + map[string]any{"name": "a"}, + }, + } + + result := MergeMaps(base, override) + + // Expected: list should be completely replaced + resultList := result["list"].([]any) + if len(resultList) != 1 { + t.Fatalf("Expected list length 1, got %d", len(resultList)) + } + + elem := resultList[0].(map[string]any) + if elem["name"] != "a" { + t.Errorf("Expected name 'a', got %v", elem["name"]) + } + // values field should not exist since it was not in override + if _, exists := elem["values"]; exists { + t.Errorf("values field should not exist in result, but got: %v", elem["values"]) + } + }) + + t.Run("empty array should replace non-empty array", func(t *testing.T) { + base := map[string]interface{}{ + "list": []any{ + map[string]any{"name": "dummy", "values": []any{1, 2}}, + }, + } + + override := map[string]interface{}{ + "list": []any{}, + } + + result := MergeMaps(base, override) + + // Expected: list should be empty + resultList := result["list"].([]any) + if len(resultList) != 0 { + t.Errorf("Expected empty list, got length %d: %+v", len(resultList), resultList) + } + }) + + t.Run("nested empty array should replace non-empty nested array", func(t *testing.T) { + base := map[string]interface{}{ + "list": []any{ + map[string]any{"name": "dummy", "values": []any{1, 2}}, + }, + } + + override := map[string]interface{}{ + "list": []any{ + map[string]any{"name": "a", "values": []any{}}, + }, + } + + result := MergeMaps(base, override) + + // Expected: list should be replaced with one element that has empty values + resultList := result["list"].([]any) + if len(resultList) != 1 { + t.Fatalf("Expected list length 1, got %d", len(resultList)) + } + + elem := resultList[0].(map[string]any) + if elem["name"] != "a" { + t.Errorf("Expected name 'a', got %v", elem["name"]) + } + + values := elem["values"].([]any) + if len(values) != 0 { + t.Errorf("Expected empty values array, got length %d: %+v", len(values), values) + } + }) +} diff --git a/pkg/state/create.go b/pkg/state/create.go index 3e68b1eb..c3f77d34 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -412,9 +412,9 @@ func (c *StateCreator) loadEnvValues(st *HelmState, name string, failOnMissingEn if overrode != nil { intOverrodeEnv := *newEnv - // Use MergeMaps instead of mergo.Merge to properly handle array merging element-by-element - // This fixes issue #2281 where arrays were being replaced entirely instead of merged - intOverrodeEnv.Values = maputil.MergeMaps(intOverrodeEnv.Values, overrode.Values) + // Use MergeMapsWithArrayMerge instead of mergo.Merge to properly handle array merging element-by-element + // This fixes issue #2281 where arrays from --state-values-set were being replaced entirely instead of merged + intOverrodeEnv.Values = maputil.MergeMapsWithArrayMerge(intOverrodeEnv.Values, overrode.Values) newEnv = &intOverrodeEnv } diff --git a/pkg/state/state.go b/pkg/state/state.go index 634f1e6a..0496e604 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -150,6 +150,9 @@ type SubHelmfileSpec struct { // SubhelmfileEnvironmentSpec is the environment spec for a subhelmfile type SubhelmfileEnvironmentSpec struct { OverrideValues []any `yaml:"values,omitempty"` + // CLISet is set internally to pass CLI overrides from --state-values-set + // This is not part of the YAML spec + CLISet map[string]any `yaml:"-"` } // HelmSpec to defines helmDefault values