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 <amenon@canarytechnologies.com>
This commit is contained in:
parent
212d1fba77
commit
5355ee4210
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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"])
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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...)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)"
|
||||
|
|
@ -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
|
||||
|
|
@ -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 }}
|
||||
|
|
@ -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
|
||||
|
|
@ -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"]
|
||||
Loading…
Reference in New Issue