mirror of
https://github.com/helmfile/helmfile.git
synced 2026-10-19 00:22:18 +02:00
fix: array merge regression - layer arrays now replace defaults (#2367)
* 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> * fix: use separate CLIOverrides field for element-by-element array merging The previous approach using ArrayMergeStrategySparse detection didn't work for --state-values-set array[0]=value because setting index 0 produces no nils in the array. This fix adds a CLIOverrides field to Environment that keeps CLI values separate from layer values. CLI overrides are merged last using ArrayMergeStrategyMerge (always element-by-element), while layer values use the default strategy (arrays replace). This ensures: - --state-values-set array[0]=x only changes index 0, preserving other elements - Layer/environment file arrays still replace base arrays entirely - Issue #2281 fix is preserved (--state-values-set array[1].field=x works) Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: correct comment about array merge strategy in test Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: propagate Defaults in multi-part helmfiles and fix merge order - Add Defaults field merging from ctxEnv to preserve base values across helmfile parts separated by --- - Fix merge order: current part values now correctly override previous parts (was reversed, causing older values to win) - Update 147 snapshot test files for new Environment log format with CLIOverrides field This completes the fix for issue #2353 by ensuring: 1. Layer arrays replace entirely (not element-by-element merge) 2. CLI --state-values-set sparse arrays still merge element-by-element 3. Multi-part helmfiles properly inherit and override values Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: address Copilot review comments - Initialize EmptyEnvironment with empty maps to match New() constructor - Update test comment to accurately describe ArrayMergeStrategySparse Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: ensure templates access merged values via .Environment.Values This commit fixes a regression in the CLIOverrides integration where templates accessing .Environment.Values couldn't see CLI override values. Changes: - Remove CLIOverrides-into-Values merge from Merge() to keep proper layering order (Defaults → Values → CLIOverrides) in GetMergedValues() - Update NewEnvironmentTemplateData to set envCopy.Values to the merged values, ensuring templates see the same values via both .Values and .Environment.Values This ensures: - Issue #2353: Layer arrays still replace entirely (Sparse strategy) - Issue #2281: CLI sparse arrays still merge element-by-element - Templates can access CLI overrides via .Environment.Values Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * docs: improve mergeSlices documentation per Copilot review Address Copilot review comments on PR #2367: - Document empty array edge case: explicitly setting [] clears base array - Document recursive strategy propagation for nested map merging - Add comprehensive behavior description for all array merge strategies Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: use merged values when rendering environment value files Environment value files (*.yaml.gotmpl) can reference CLI values via .Values. Previously, only env.Values was passed to template rendering, which didn't include CLIOverrides. Now we call env.GetMergedValues() to get Defaults + Values + CLIOverrides before rendering, so templates can access CLI values like: --state-values-set foo=bar This fixes the state-values-set-cli-args-in-environments integration test. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> --------- Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
This commit is contained in:
Aditya Menon
+287
-11
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user