mirror of
https://github.com/helmfile/helmfile.git
synced 2026-10-01 19:00:08 +02:00
* 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>
208 lines
4.6 KiB
Go
208 lines
4.6 KiB
Go
package environment
|
|
|
|
import (
|
|
"testing"
|
|
|
|
"github.com/google/go-cmp/cmp"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestMerge_Issues(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
dst *Environment
|
|
src *Environment
|
|
expected map[string]any
|
|
}{
|
|
{
|
|
name: "OverwriteNilValue_Issue1150",
|
|
dst: &Environment{
|
|
Name: "dst",
|
|
Values: map[string]any{
|
|
"components": map[string]any{
|
|
"etcd-operator": nil,
|
|
},
|
|
},
|
|
Defaults: nil,
|
|
},
|
|
src: &Environment{
|
|
Name: "src",
|
|
Values: map[string]any{
|
|
"components": map[string]any{
|
|
"etcd-operator": map[string]any{
|
|
"version": "0.10.3",
|
|
},
|
|
},
|
|
},
|
|
Defaults: nil,
|
|
},
|
|
expected: map[string]any{
|
|
"components": map[string]any{
|
|
"etcd-operator": map[string]any{
|
|
"version": "0.10.3",
|
|
},
|
|
},
|
|
},
|
|
},
|
|
{
|
|
name: "OverwriteWithNilValue_Issue1154",
|
|
dst: &Environment{
|
|
Name: "dst",
|
|
Values: map[string]any{
|
|
"components": map[string]any{
|
|
"etcd-operator": map[string]any{
|
|
"version": "0.10.0",
|
|
},
|
|
},
|
|
},
|
|
Defaults: nil,
|
|
},
|
|
src: &Environment{
|
|
Name: "src",
|
|
Values: map[string]any{
|
|
"components": map[string]any{
|
|
"etcd-operator": map[string]any{
|
|
"version": "0.10.3",
|
|
},
|
|
"prometheus": nil,
|
|
},
|
|
},
|
|
Defaults: nil,
|
|
},
|
|
expected: map[string]any{
|
|
"components": map[string]any{
|
|
"etcd-operator": map[string]any{
|
|
"version": "0.10.3",
|
|
},
|
|
"prometheus": nil,
|
|
},
|
|
},
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
merged, err := tt.dst.Merge(tt.src)
|
|
require.NoError(t, err)
|
|
|
|
actual := merged.Values
|
|
assert.Empty(t, cmp.Diff(tt.expected, actual), "unexpected diff")
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestNew(t *testing.T) {
|
|
envName := "test"
|
|
env := New(envName)
|
|
|
|
require.Equal(t, envName, env.Name, "environment name should be %s, but got %s", envName, env.Name)
|
|
}
|
|
|
|
func TestEnvironment_DeepCopy(t *testing.T) {
|
|
env := &Environment{
|
|
Name: "test",
|
|
Values: map[string]any{
|
|
"foo": "bar",
|
|
},
|
|
Defaults: map[string]any{
|
|
"baz": "qux",
|
|
},
|
|
}
|
|
|
|
copy := env.DeepCopy()
|
|
|
|
assert.Equal(t, env.Name, copy.Name)
|
|
assert.Equal(t, env.Values, copy.Values)
|
|
assert.Equal(t, env.Defaults, copy.Defaults)
|
|
|
|
copy.Values["foo"] = "modified"
|
|
assert.NotEqual(t, env.Values["foo"], copy.Values["foo"])
|
|
}
|
|
|
|
func TestEnvironment_GetMergedValues(t *testing.T) {
|
|
env := &Environment{
|
|
Name: "test",
|
|
Values: map[string]any{
|
|
"foo": "bar",
|
|
},
|
|
Defaults: map[string]any{
|
|
"baz": "qux",
|
|
},
|
|
}
|
|
|
|
mergedValues, err := env.GetMergedValues()
|
|
require.NoError(t, err)
|
|
|
|
expected := map[string]any{
|
|
"foo": "bar",
|
|
"baz": "qux",
|
|
}
|
|
|
|
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"])
|
|
}
|