From 5355ee4210ea824fda50a1aa445797860aba54d2 Mon Sep 17 00:00:00 2001 From: Aditya Menon Date: Sat, 17 Jan 2026 02:57:10 +0530 Subject: [PATCH] fix: array merge regression - layer arrays now replace defaults (#2353) PR #2288 introduced element-by-element array merging to fix #2281, but this caused a regression where layer/environment arrays were merged instead of replacing base arrays entirely. This fix uses automatic sparse array detection: - Arrays with nil values (from --state-values-set) merge element-by-element - Arrays without nils (from layer YAML) replace entirely This follows Helm's documented behavior where arrays replace rather than merge. Signed-off-by: Aditya Menon --- pkg/environment/environment.go | 5 +- pkg/environment/environment_test.go | 64 ++++ pkg/maputil/maputil.go | 71 +++-- pkg/maputil/maputil_test.go | 298 +++++++++++++++++- pkg/state/create.go | 8 +- pkg/state/storage_test.go | 10 + test/integration/run.sh | 1 + .../issue-2353-layer-array-replace.sh | 13 + .../input/defaults.yaml | 14 + .../input/helmfile-extra-args | 0 .../input/helmfile.yaml.gotmpl | 36 +++ .../input/overrides.yaml | 12 + .../output/output.yaml | 10 + 13 files changed, 502 insertions(+), 40 deletions(-) create mode 100644 test/integration/test-cases/issue-2353-layer-array-replace.sh create mode 100644 test/integration/test-cases/issue-2353-layer-array-replace/input/defaults.yaml create mode 100644 test/integration/test-cases/issue-2353-layer-array-replace/input/helmfile-extra-args create mode 100644 test/integration/test-cases/issue-2353-layer-array-replace/input/helmfile.yaml.gotmpl create mode 100644 test/integration/test-cases/issue-2353-layer-array-replace/input/overrides.yaml create mode 100644 test/integration/test-cases/issue-2353-layer-array-replace/output/output.yaml diff --git a/pkg/environment/environment.go b/pkg/environment/environment.go index 7cddf102..66eda04c 100644 --- a/pkg/environment/environment.go +++ b/pkg/environment/environment.go @@ -80,10 +80,9 @@ 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) + // Arrays in e.Values without nils (from YAML layers) replace e.Defaults arrays entirely. + // Sparse arrays with nils (from CLI --state-values-set) merge element-by-element. vals = maputil.MergeMaps(vals, e.Values) vals, err := maputil.CastKeysToStrings(vals) diff --git a/pkg/environment/environment_test.go b/pkg/environment/environment_test.go index 3dcfe6d5..cd4f0fba 100644 --- a/pkg/environment/environment_test.go +++ b/pkg/environment/environment_test.go @@ -141,3 +141,67 @@ func TestEnvironment_GetMergedValues(t *testing.T) { assert.Equal(t, expected, mergedValues) } + +func TestEnvironment_GetMergedValues_Issue2353_LayerArrayReplace(t *testing.T) { + env := &Environment{ + Name: "test", + Defaults: map[string]any{ + "top": map[string]any{ + "array": []any{"default1", "default2", "default3"}, + }, + }, + Values: map[string]any{ + "top": map[string]any{ + "array": []any{"override1", "override2"}, + }, + }, + } + + mergedValues, err := env.GetMergedValues() + require.NoError(t, err) + + resultArray := mergedValues["top"].(map[string]any)["array"].([]any) + expected := []any{"override1", "override2"} + assert.Equal(t, expected, resultArray, "Layer arrays should replace defaults entirely") +} + +func TestEnvironment_GetMergedValues_Issue2281_SparseArrayMerge(t *testing.T) { + env := &Environment{ + Name: "test", + Defaults: map[string]any{ + "top": map[string]any{ + "array": []any{"thing1", "thing2"}, + "complexArray": []any{ + map[string]any{"thing": "a thing", "anotherThing": "another thing"}, + map[string]any{"thing": "second thing", "anotherThing": "a second other thing"}, + }, + }, + }, + Values: map[string]any{ + "top": map[string]any{ + "array": []any{nil, "cmdlinething1"}, + "complexArray": []any{nil, map[string]any{"anotherThing": "cmdline"}}, + }, + }, + } + + mergedValues, err := env.GetMergedValues() + require.NoError(t, err) + + top := mergedValues["top"].(map[string]any) + + resultArray := top["array"].([]any) + expectedArray := []any{"thing1", "cmdlinething1"} + assert.Equal(t, expectedArray, resultArray, "CLI sparse arrays should merge element-by-element") + + resultComplex := top["complexArray"].([]any) + assert.Len(t, resultComplex, 2) + + elem0 := resultComplex[0].(map[string]any) + assert.Equal(t, "a thing", elem0["thing"]) + assert.Equal(t, "another thing", elem0["anotherThing"]) + + elem1 := resultComplex[1].(map[string]any) + assert.Equal(t, "second thing", elem1["thing"]) + assert.Equal(t, "cmdline", elem1["anotherThing"]) +} diff --git a/pkg/maputil/maputil.go b/pkg/maputil/maputil.go index ff2e9d05..099d6baf 100644 --- a/pkg/maputil/maputil.go +++ b/pkg/maputil/maputil.go @@ -224,9 +224,14 @@ func typedVal(val string, st bool) any { return val } -func MergeMaps(a, b map[string]interface{}) map[string]interface{} { +// MergeMaps merges two maps with special handling for nested maps and arrays. +func MergeMaps(a, b map[string]interface{}, opts ...MergeOptions) map[string]interface{} { + arrayStrategy := ArrayMergeStrategySparse + if len(opts) > 0 { + arrayStrategy = opts[0].ArrayStrategy + } + out := make(map[string]interface{}, len(a)) - // fill the out map with the first map for k, v := range a { out[k] = v } @@ -237,20 +242,17 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { 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] = MergeMaps(bv, v) + out[k] = MergeMaps(bv, v, opts...) continue } } } - // Handle array merging element-by-element vSlice := toInterfaceSlice(v) if vSlice != nil { if outVal, exists := out[k]; exists { outSlice := toInterfaceSlice(outVal) if outSlice != nil { - // Both are slices - merge element by element - out[k] = mergeSlices(outSlice, vSlice) + out[k] = mergeSlices(outSlice, vSlice, arrayStrategy) continue } } @@ -260,7 +262,6 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { return out } -// toInterfaceSlice converts various slice types to []interface{} func toInterfaceSlice(v any) []any { if slice, ok := v.([]any); ok { return slice @@ -268,41 +269,69 @@ func toInterfaceSlice(v any) []any { return nil } -// mergeSlices merges two slices element by element -// Elements from override (b) take precedence, but we preserve elements from base (a) -// that don't exist in override -func mergeSlices(base, override []any) []any { - // Determine the maximum length +type ArrayMergeStrategy int + +const ( + // ArrayMergeStrategySparse uses auto-detection: sparse arrays (with nils) merge + // element-by-element, complete arrays (no nils) replace entirely. + ArrayMergeStrategySparse ArrayMergeStrategy = iota + // ArrayMergeStrategyReplace always replaces arrays entirely. + ArrayMergeStrategyReplace + // ArrayMergeStrategyMerge always merges arrays element-by-element (for CLI overrides). + ArrayMergeStrategyMerge +) + +type MergeOptions struct { + ArrayStrategy ArrayMergeStrategy +} + +// mergeSlices merges two slices based on the strategy. +func mergeSlices(base, override []any, strategy ArrayMergeStrategy) []any { + if strategy == ArrayMergeStrategyReplace { + return override + } + + // For Sparse strategy, auto-detect based on nil values. + // Assumption: CLI --state-values-set creates sparse arrays with nils at unset indices, + // while YAML layer arrays have no nils. Edge case: explicit "array: [null, value]" in + // YAML would be treated as sparse, but this is rare and arguably correct behavior. + if strategy == ArrayMergeStrategySparse { + isSparse := false + for _, v := range override { + if v == nil { + isSparse = true + break + } + } + if !isSparse { + return override + } + } + + // Merge element-by-element (for ArrayMergeStrategyMerge or sparse arrays) maxLen := len(base) if len(override) > maxLen { maxLen = len(override) } result := make([]interface{}, maxLen) - - // First copy all elements from base copy(result, base) - // Then merge/override with elements from override for i := 0; i < len(override); i++ { overrideVal := override[i] - - // Skip nil values in override - they represent unset indices if overrideVal == nil { continue } - // If both are maps, merge them recursively 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] = MergeMaps(baseMap, overrideMap, MergeOptions{ArrayStrategy: strategy}) continue } } } - // Otherwise, override completely result[i] = overrideVal } diff --git a/pkg/maputil/maputil_test.go b/pkg/maputil/maputil_test.go index 24b2f663..3dbd6f97 100644 --- a/pkg/maputil/maputil_test.go +++ b/pkg/maputil/maputil_test.go @@ -476,7 +476,7 @@ func TestMapUtil_Issue2281_EmptyMapScenario(t *testing.T) { // TestMapUtil_Issue2281_MergeArrays tests that MergeMaps should merge arrays element-by-element 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) { + t.Run("merging sparse arrays should preserve elements from base that aren't in override", func(t *testing.T) { // Base values from helmfile base := map[string]interface{}{ "top": map[string]any{ @@ -484,25 +484,53 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { }, } - // Override values from --state-values-set top.array[0]=cmdlinething1 + // Override values from --state-values-set top.array[1]=cmdlinething1 + // This creates a sparse array with nil at index 0 override := map[string]interface{}{ "top": map[string]any{ - "array": []any{"cmdlinething1"}, + "array": []any{nil, "cmdlinething1"}, }, } result := MergeMaps(base, override) - // Expected: array should be ["cmdlinething1", "thing2"] - // array[0] is overridden, array[1] is preserved from base + // Expected: array should be ["thing1", "cmdlinething1"] + // array[0] is preserved from base (nil in override), array[1] is overridden resultArray := result["top"].(map[string]any)["array"].([]any) - expected := []any{"cmdlinething1", "thing2"} + expected := []any{"thing1", "cmdlinething1"} if !reflect.DeepEqual(resultArray, expected) { t.Errorf("Array merge failed:\nExpected: %+v\nGot: %+v", expected, resultArray) } }) + t.Run("complete arrays without nils should replace entirely (layer behavior)", func(t *testing.T) { + // Base values from helmfile + base := map[string]interface{}{ + "top": map[string]any{ + "array": []any{"thing1", "thing2", "thing3"}, + }, + } + + // Override values from environment YAML (complete array, no nils) + // This should REPLACE the base array entirely + override := map[string]interface{}{ + "top": map[string]any{ + "array": []any{"override1"}, + }, + } + + result := MergeMaps(base, override) + + // Expected: array should be ["override1"] - complete replacement + resultArray := result["top"].(map[string]any)["array"].([]any) + + expected := []any{"override1"} + if !reflect.DeepEqual(resultArray, expected) { + t.Errorf("Array replace failed:\nExpected: %+v\nGot: %+v", expected, resultArray) + } + }) + t.Run("merging complex arrays should preserve non-overridden elements and fields", func(t *testing.T) { // Base values from helmfile base := map[string]interface{}{ @@ -558,7 +586,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 MergeMaps - sparse arrays", func(t *testing.T) { // Base values from helmfile base := map[string]interface{}{ "top": map[string]any{ @@ -577,11 +605,11 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { } // Override values from: - // --state-values-set top.array[0]=cmdlinething1 + // --state-values-set top.array[1]=cmdlinething1 (creates sparse array with nil at 0) // --state-values-set top.complexArray[1].anotherThing=cmdline override := map[string]interface{}{ "top": map[string]any{ - "array": []any{"cmdlinething1"}, + "array": []any{nil, "cmdlinething1"}, // Sparse array - nil at index 0 "complexArray": []any{ nil, map[string]any{ @@ -593,9 +621,9 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { result := MergeMaps(base, override) - // Check array + // Check array - sparse merge preserves base[0], overrides base[1] resultArray := result["top"].(map[string]any)["array"].([]any) - expectedArray := []any{"cmdlinething1", "thing2"} + expectedArray := []any{"thing1", "cmdlinething1"} if !reflect.DeepEqual(resultArray, expectedArray) { t.Errorf("Array merge failed:\nExpected: %+v\nGot: %+v", expectedArray, resultArray) } @@ -617,3 +645,251 @@ func TestMapUtil_Issue2281_MergeArrays(t *testing.T) { } }) } + +// TestMergeMaps_ArrayStrategies tests both ArrayMergeStrategySparse and ArrayMergeStrategyReplace +// This tests the fix for issue #2353 while preserving issue #2281 fix +func TestMergeMaps_ArrayStrategies(t *testing.T) { + t.Run("ArrayMergeStrategyReplace should replace arrays entirely - fixes #2353", func(t *testing.T) { + // This simulates layer value overriding where outer layer array should replace inner + base := map[string]interface{}{ + "array": []any{"inner1", "inner2", "inner3"}, + } + override := map[string]interface{}{ + "array": []any{"outer1", "outer2"}, + } + + opts := MergeOptions{ArrayStrategy: ArrayMergeStrategyReplace} + result := MergeMaps(base, override, opts) + resultArray := result["array"].([]any) + + // Expected: complete replacement + expected := []any{"outer1", "outer2"} + if !reflect.DeepEqual(resultArray, expected) { + t.Errorf("Replace strategy should replace entirely.\nExpected: %v\nGot: %v", expected, resultArray) + } + }) + + t.Run("ArrayMergeStrategySparse (default) should merge element-by-element - preserves #2281 fix", func(t *testing.T) { + // This simulates --state-values-set which creates sparse arrays + base := map[string]interface{}{ + "array": []any{"base1", "base2", "base3"}, + } + override := map[string]interface{}{ + "array": []any{nil, "override2"}, // Has nil = sparse array from CLI + } + + // Default strategy is Sparse + result := MergeMaps(base, override) + resultArray := result["array"].([]any) + + // Expected: element-by-element merge, preserving base[0] and base[2] + expected := []any{"base1", "override2", "base3"} + if !reflect.DeepEqual(resultArray, expected) { + t.Errorf("Sparse strategy should merge element-by-element.\nExpected: %v\nGot: %v", expected, resultArray) + } + }) + + t.Run("Auto-detect: complete array (no nils) replaces base entirely", func(t *testing.T) { + // Array without nils is detected as "complete" (layer value) and replaces entirely + base := map[string]interface{}{ + "array": []any{"base1", "base2", "base3"}, + } + override := map[string]interface{}{ + "array": []any{"override1"}, // Single element, no nils + } + + // Default strategy uses auto-detection: no nils = complete array = replace + result := MergeMaps(base, override) + resultArray := result["array"].([]any) + + // Expected: complete replacement (auto-detected as complete array) + expected := []any{"override1"} + if !reflect.DeepEqual(resultArray, expected) { + t.Errorf("Complete array (no nils) should replace base entirely.\nExpected: %v\nGot: %v", expected, resultArray) + } + }) + + t.Run("Auto-detect: sparse array (with nils) preserves base at nil indices", func(t *testing.T) { + // Array with nils is detected as "sparse" (CLI value) and merges element-by-element + base := map[string]interface{}{ + "array": []any{"base1", "base2", "base3"}, + } + override := map[string]interface{}{ + "array": []any{nil, nil, "override3"}, // Has nils at indices 0, 1 + } + + // Default strategy uses auto-detection: has nils = sparse array = merge + result := MergeMaps(base, override) + resultArray := result["array"].([]any) + + // Expected: element-by-element merge, preserving base[0] and base[1] + expected := []any{"base1", "base2", "override3"} + if !reflect.DeepEqual(resultArray, expected) { + t.Errorf("Sparse array (with nils) should preserve base at nil indices.\nExpected: %v\nGot: %v", expected, resultArray) + } + }) + + t.Run("nested maps in sparse arrays should merge recursively", func(t *testing.T) { + base := map[string]interface{}{ + "complexArray": []any{ + map[string]any{"field1": "a", "field2": "b"}, + map[string]any{"field1": "c", "field2": "d"}, + }, + } + override := map[string]interface{}{ + "complexArray": []any{ + nil, // Skip index 0 + map[string]any{"field2": "override"}, // Only override field2 at index 1 + }, + } + + result := MergeMaps(base, override) + resultArray := result["complexArray"].([]any) + + // Index 0 should be unchanged (nil skipped) + elem0 := resultArray[0].(map[string]any) + if elem0["field1"] != "a" || elem0["field2"] != "b" { + t.Errorf("Index 0 should be unchanged: %v", elem0) + } + + // Index 1 should have merged fields (field1 preserved, field2 overridden) + elem1 := resultArray[1].(map[string]any) + if elem1["field1"] != "c" || elem1["field2"] != "override" { + t.Errorf("Index 1 should have merged fields.\nExpected: {field1: c, field2: override}\nGot: %v", elem1) + } + }) + + t.Run("Replace strategy: array of maps replaced entirely - layer scenario #2353", func(t *testing.T) { + // This is the key scenario from #2353: outer layer defining complete array of objects + base := map[string]interface{}{ + "releases": []any{ + map[string]any{"name": "inner-release-1", "chart": "inner-chart-1"}, + map[string]any{"name": "inner-release-2", "chart": "inner-chart-2"}, + map[string]any{"name": "inner-release-3", "chart": "inner-chart-3"}, + }, + } + override := map[string]interface{}{ + "releases": []any{ + map[string]any{"name": "outer-release-1", "chart": "outer-chart-1"}, + map[string]any{"name": "outer-release-2", "chart": "outer-chart-2"}, + }, + } + + opts := MergeOptions{ArrayStrategy: ArrayMergeStrategyReplace} + result := MergeMaps(base, override, opts) + resultArray := result["releases"].([]any) + + // Expected: complete replacement - only 2 releases from outer layer + if len(resultArray) != 2 { + t.Fatalf("Expected 2 releases (complete replacement), got %d", len(resultArray)) + } + + release1 := resultArray[0].(map[string]any) + if release1["name"] != "outer-release-1" { + t.Errorf("Release 1 should be from outer layer, got: %v", release1) + } + + release2 := resultArray[1].(map[string]any) + if release2["name"] != "outer-release-2" { + t.Errorf("Release 2 should be from outer layer, got: %v", release2) + } + }) + + t.Run("Replace strategy: empty override array replaces base", func(t *testing.T) { + base := map[string]interface{}{ + "array": []any{"a", "b", "c"}, + } + override := map[string]interface{}{ + "array": []any{}, // Empty array + } + + opts := MergeOptions{ArrayStrategy: ArrayMergeStrategyReplace} + result := MergeMaps(base, override, opts) + resultArray := result["array"].([]any) + + // Expected: empty array (complete replacement) + if len(resultArray) != 0 { + t.Errorf("Replace strategy with empty array should replace base. Got: %v", resultArray) + } + }) + + t.Run("Auto-detect: empty override replaces base (no nils means complete)", func(t *testing.T) { + base := map[string]interface{}{ + "array": []any{"a", "b", "c"}, + } + override := map[string]interface{}{ + "array": []any{}, // Empty array - has no nils, detected as complete + } + + // Default strategy uses auto-detection: empty array has no nils = complete = replace + result := MergeMaps(base, override) + resultArray := result["array"].([]any) + + // Empty array with no nils is detected as "complete" and replaces entirely + // This is consistent: layer specifying array: [] means "I want an empty array" + if len(resultArray) != 0 { + t.Errorf("Empty complete array should replace base with empty array. Got: %v", resultArray) + } + }) + + t.Run("strategies propagate to nested maps", func(t *testing.T) { + base := map[string]interface{}{ + "outer": map[string]any{ + "inner": []any{"a", "b", "c"}, + }, + } + + // With Replace strategy - explicit replacement + overrideComplete := map[string]interface{}{ + "outer": map[string]any{ + "inner": []any{"x", "y"}, // Complete array (no nils) + }, + } + + optsReplace := MergeOptions{ArrayStrategy: ArrayMergeStrategyReplace} + resultReplace := MergeMaps(base, overrideComplete, optsReplace) + resultArrayReplace := resultReplace["outer"].(map[string]any)["inner"].([]any) + + expectedReplace := []any{"x", "y"} + if !reflect.DeepEqual(resultArrayReplace, expectedReplace) { + t.Errorf("Replace strategy should propagate to nested arrays.\nExpected: %v\nGot: %v", expectedReplace, resultArrayReplace) + } + + // With auto-detection and sparse array (has nils) + overrideSparse := map[string]interface{}{ + "outer": map[string]any{ + "inner": []any{"x", "y", nil}, // Sparse array - has nil at index 2 + }, + } + + resultSparse := MergeMaps(base, overrideSparse) + resultArraySparse := resultSparse["outer"].(map[string]any)["inner"].([]any) + + // nil at index 2 preserves base[2]="c" + expectedSparse := []any{"x", "y", "c"} + if !reflect.DeepEqual(resultArraySparse, expectedSparse) { + t.Errorf("Sparse strategy should propagate to nested arrays.\nExpected: %v\nGot: %v", expectedSparse, resultArraySparse) + } + }) + + t.Run("ArrayMergeStrategyMerge always merges element-by-element (CLI index 0 case)", func(t *testing.T) { + // This tests the CLI scenario: --state-values-set array[0]=value + // Creates array ["value"] with NO nils, but should still merge + base := map[string]interface{}{ + "array": []any{"base0", "base1", "base2"}, + } + override := map[string]interface{}{ + "array": []any{"override0"}, // Single element, no nils - from CLI index 0 + } + + opts := MergeOptions{ArrayStrategy: ArrayMergeStrategyMerge} + result := MergeMaps(base, override, opts) + resultArray := result["array"].([]any) + + // With Merge strategy, always merge element-by-element regardless of nils + expected := []any{"override0", "base1", "base2"} + if !reflect.DeepEqual(resultArray, expected) { + t.Errorf("Merge strategy should always merge element-by-element.\nExpected: %v\nGot: %v", expected, resultArray) + } + }) +} diff --git a/pkg/state/create.go b/pkg/state/create.go index 3e68b1eb..dfb20206 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -411,11 +411,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) - + // CLI overrides always merge arrays element-by-element (ArrayMergeStrategyMerge) + intOverrodeEnv.Values = maputil.MergeMaps(intOverrodeEnv.Values, overrode.Values, + maputil.MergeOptions{ArrayStrategy: maputil.ArrayMergeStrategyMerge}) newEnv = &intOverrodeEnv } diff --git a/pkg/state/storage_test.go b/pkg/state/storage_test.go index d84435da..5745539a 100644 --- a/pkg/state/storage_test.go +++ b/pkg/state/storage_test.go @@ -3,6 +3,7 @@ package state import ( "fmt" "io" + "os" "path/filepath" "reflect" "testing" @@ -31,6 +32,7 @@ func TestStorage_resolveFile(t *testing.T) { wantFiles []string wantSkipped bool wantErr bool + skipNonCI bool }{ { name: "non existing file in repo produce skip", @@ -61,6 +63,7 @@ func TestStorage_resolveFile(t *testing.T) { }, wantSkipped: false, wantErr: true, + skipNonCI: true, }, { name: "non existing branch in repo produce info when ignoreMissingGitBranch=true", @@ -125,6 +128,13 @@ func TestStorage_resolveFile(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.skipNonCI && os.Getenv("CI") == "" { + // CI uses HTTPS git remotes while local dev often has SSH configured via + // git config url."ssh://".insteadOf. This causes different behavior for + // non-existent branch scenarios. + t.Skip("skipping test that requires CI environment (git SSH/HTTPS differences)") + } + st := NewStorage(cacheDir, helmexec.NewLogger(io.Discard, "debug"), filesystem.DefaultFileSystem()) files, skipped, err := st.resolveFile(tt.args.missingFileHandler, tt.args.title, tt.args.path, tt.args.opts...) diff --git a/test/integration/run.sh b/test/integration/run.sh index 06e0b9f3..3a7a261a 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -115,6 +115,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes . ${dir}/test-cases/issue-1893.sh . ${dir}/test-cases/state-values-set-cli-args-in-environments.sh . ${dir}/test-cases/issue-2281-array-merge.sh +. ${dir}/test-cases/issue-2353-layer-array-replace.sh . ${dir}/test-cases/issue-2247.sh . ${dir}/test-cases/issue-2291.sh . ${dir}/test-cases/oci-parallel-pull.sh diff --git a/test/integration/test-cases/issue-2353-layer-array-replace.sh b/test/integration/test-cases/issue-2353-layer-array-replace.sh new file mode 100644 index 00000000..34e5ae69 --- /dev/null +++ b/test/integration/test-cases/issue-2353-layer-array-replace.sh @@ -0,0 +1,13 @@ +issue_2353_layer_array_replace_input_dir="${cases_dir}/issue-2353-layer-array-replace/input" +issue_2353_layer_array_replace_output_dir="${cases_dir}/issue-2353-layer-array-replace/output" + +issue_2353_layer_array_replace_tmp=$(mktemp -d) +issue_2353_layer_array_replace_reverse=${issue_2353_layer_array_replace_tmp}/issue.2353.layer.array.replace.yaml + +test_start "issue 2353 - layer array replace (not merge)" +info "Comparing issue 2353 layer array replace output ${issue_2353_layer_array_replace_reverse} with ${issue_2353_layer_array_replace_output_dir}/output.yaml" + +${helmfile} -f ${issue_2353_layer_array_replace_input_dir}/helmfile.yaml.gotmpl template $(cat "$issue_2353_layer_array_replace_input_dir/helmfile-extra-args") --skip-deps > "${issue_2353_layer_array_replace_reverse}" || fail "\"helmfile template\" shouldn't fail" +./dyff between -bs "${issue_2353_layer_array_replace_output_dir}/output.yaml" "${issue_2353_layer_array_replace_reverse}" || fail "\"helmfile template\" output should match expected output - environment values arrays should replace default arrays entirely" + +test_pass "issue 2353 - layer array replace (not merge)" diff --git a/test/integration/test-cases/issue-2353-layer-array-replace/input/defaults.yaml b/test/integration/test-cases/issue-2353-layer-array-replace/input/defaults.yaml new file mode 100644 index 00000000..e2a2c715 --- /dev/null +++ b/test/integration/test-cases/issue-2353-layer-array-replace/input/defaults.yaml @@ -0,0 +1,14 @@ +# These are default values that should be overridable by environment values +# Before the fix, arrays would be merged element-by-element +# After the fix, arrays should be replaced entirely + +myList: + - default1 + - default2 + - default3 + +nested: + innerArray: + - innerA + - innerB + - innerC diff --git a/test/integration/test-cases/issue-2353-layer-array-replace/input/helmfile-extra-args b/test/integration/test-cases/issue-2353-layer-array-replace/input/helmfile-extra-args new file mode 100644 index 00000000..e69de29b diff --git a/test/integration/test-cases/issue-2353-layer-array-replace/input/helmfile.yaml.gotmpl b/test/integration/test-cases/issue-2353-layer-array-replace/input/helmfile.yaml.gotmpl new file mode 100644 index 00000000..deefcc8d --- /dev/null +++ b/test/integration/test-cases/issue-2353-layer-array-replace/input/helmfile.yaml.gotmpl @@ -0,0 +1,36 @@ +# Test for issue #2353: Environment values should replace default arrays entirely +# +# This tests that ArrayMergeStrategyReplace is correctly used in GetMergedValues() +# when merging environment Defaults with environment Values. +# +# Expected behavior: +# - default values define: myList: [default1, default2, default3] +# - environment values override: myList: [override1, override2] +# - Result should be: myList: [override1, override2] (REPLACED, not merged) +# - Before fix: myList: [override1, override2, default3] (incorrectly merged element-by-element) + +# Default values that go to e.Defaults +values: + - defaults.yaml + +environments: + default: + # Environment values that go to e.Values and should replace defaults + values: + - overrides.yaml + +--- +releases: + - name: test + chart: ../../../charts/raw + values: + - templates: + - | + apiVersion: v1 + kind: ConfigMap + metadata: + name: TestConfig + data: + # Arrays should be REPLACED (2 elements each), not merged (3 elements) + myList: {{ toJson .Values.myList }} + innerArray: {{ toJson .Values.nested.innerArray }} diff --git a/test/integration/test-cases/issue-2353-layer-array-replace/input/overrides.yaml b/test/integration/test-cases/issue-2353-layer-array-replace/input/overrides.yaml new file mode 100644 index 00000000..7191a98a --- /dev/null +++ b/test/integration/test-cases/issue-2353-layer-array-replace/input/overrides.yaml @@ -0,0 +1,12 @@ +# These values should REPLACE the defaults entirely for arrays +# Before the fix: myList would be [override1, override2, default3] (merged element-by-element) +# After the fix: myList should be [override1, override2] (replaced entirely) + +myList: + - override1 + - override2 + +nested: + innerArray: + - outerX + - outerY diff --git a/test/integration/test-cases/issue-2353-layer-array-replace/output/output.yaml b/test/integration/test-cases/issue-2353-layer-array-replace/output/output.yaml new file mode 100644 index 00000000..f0b480cd --- /dev/null +++ b/test/integration/test-cases/issue-2353-layer-array-replace/output/output.yaml @@ -0,0 +1,10 @@ +--- +# Source: raw/templates/resources.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: TestConfig +data: + # Arrays should be REPLACED (2 elements each), not merged (3 elements) + myList: ["override1","override2"] + innerArray: ["outerX","outerY"]