From b9378b17f61a093d1ed3e38c0470e665bdd491e2 Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Sun, 12 Apr 2026 09:44:49 +0800 Subject: [PATCH] fix: boolean false overrides dropped in multi-document helmfiles (#2527) (#2532) * fix: environment values pollution causing boolean false overrides to be dropped (#2527) PR #2367 introduced envCopy.Values = values in NewEnvironmentTemplateData, where values = GetMergedValues() = Defaults + Values + CLIOverrides. This caused .Environment.Values to include Defaults, so when multi-part helmfiles re-assigned environment values via {{ toYaml .Environment.Values }}, Defaults values (e.g. helmDefaults.atomic: true) were written into the environment Values field. Later, GetMergedValues() applied Values over Defaults, causing the stale atomic: true to win over the correct atomic: false override. Fix: set .Environment.Values to Values + CLIOverrides only (excluding Defaults), so re-assignment patterns don't pollute the Values layer with Defaults. Signed-off-by: yxx Signed-off-by: yxxhero * fix: rename test to correctly reflect Values override Defaults precedence Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/1b251877-7050-404b-8cc7-abd6aa3ec36b Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> * test: flip regression test fixture to exercise false override (issue #2527) Agent-Logs-Url: https://github.com/helmfile/helmfile/sessions/c428fd46-b698-4e88-bff2-4c9ac72d2deb Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> --------- Signed-off-by: yxx Signed-off-by: yxxhero Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> --- pkg/environment/environment_test.go | 32 ++++++++++++++++++ pkg/state/types.go | 7 ++-- pkg/state/types_test.go | 52 +++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 pkg/state/types_test.go diff --git a/pkg/environment/environment_test.go b/pkg/environment/environment_test.go index cd4f0fba..e990995b 100644 --- a/pkg/environment/environment_test.go +++ b/pkg/environment/environment_test.go @@ -205,3 +205,35 @@ func TestEnvironment_GetMergedValues_Issue2281_SparseArrayMerge(t *testing.T) { assert.Equal(t, "second thing", elem1["thing"]) assert.Equal(t, "cmdline", elem1["anotherThing"]) } + +func TestEnvironment_GetMergedValues_Issue2527_ValuesOverrideDefaults(t *testing.T) { + // Regression test for https://github.com/helmfile/helmfile/issues/2527: + // A boolean false in Values must not be overridden by a true in Defaults. + env := &Environment{ + Name: "test", + Defaults: map[string]any{ + "helmDefaults": map[string]any{ + "atomic": true, + "wait": true, + "timeout": 300, + }, + }, + Values: map[string]any{ + "appName": "my-app", + "helmDefaults": map[string]any{ + "atomic": false, // explicit false override must survive + "wait": true, + "timeout": 300, + }, + }, + CLIOverrides: map[string]any{}, + } + + mergedValues, err := env.GetMergedValues() + require.NoError(t, err) + + hd := mergedValues["helmDefaults"].(map[string]any) + assert.Equal(t, false, hd["atomic"], "Values false should override Defaults true for atomic") + assert.Equal(t, true, hd["wait"]) + assert.Equal(t, 300, hd["timeout"]) +} diff --git a/pkg/state/types.go b/pkg/state/types.go index b210bf19..991ac73b 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -2,6 +2,7 @@ package state import ( "github.com/helmfile/helmfile/pkg/environment" + "github.com/helmfile/helmfile/pkg/maputil" ) // TemplateSpec defines the structure of a reusable and composable template for helm releases. @@ -21,11 +22,9 @@ type EnvironmentTemplateData struct { } func NewEnvironmentTemplateData(env environment.Environment, namespace string, values map[string]any) *EnvironmentTemplateData { - // Create a copy of the environment with merged values for template access. - // This ensures templates accessing .Environment.Values see the same merged values - // (Defaults + Values + CLIOverrides) as templates accessing .Values directly. envCopy := env - envCopy.Values = values + envCopy.Values = maputil.MergeMaps(env.Values, env.CLIOverrides, + maputil.MergeOptions{ArrayStrategy: maputil.ArrayMergeStrategyMerge}) d := EnvironmentTemplateData{envCopy, namespace, values, nil} d.StateValues = &d.Values return &d diff --git a/pkg/state/types_test.go b/pkg/state/types_test.go new file mode 100644 index 00000000..dd22c8d1 --- /dev/null +++ b/pkg/state/types_test.go @@ -0,0 +1,52 @@ +package state + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/helmfile/helmfile/pkg/environment" +) + +func TestNewEnvironmentTemplateData_EnvironmentValuesExcludesDefaults_Issue2527(t *testing.T) { + env := environment.Environment{ + Name: "myenv", + Defaults: map[string]any{ + "helmDefaults": map[string]any{ + "atomic": true, + "wait": true, + "timeout": 300, + }, + }, + Values: map[string]any{ + "appName": "my-app", + }, + CLIOverrides: map[string]any{ + "cliFlag": "cliValue", + }, + } + + mergedVals := map[string]any{ + "appName": "my-app", + "cliFlag": "cliValue", + "helmDefaults": map[string]any{ + "atomic": true, + "wait": true, + "timeout": 300, + }, + } + + tmplData := NewEnvironmentTemplateData(env, "ns", mergedVals) + require.NotNil(t, tmplData) + + assert.Equal(t, mergedVals, tmplData.Values, ".Values should contain full merged values") + + _, hasDefaults := tmplData.Environment.Values["helmDefaults"] + assert.False(t, hasDefaults, ".Environment.Values should NOT contain Defaults (helmDefaults)") + + assert.Equal(t, "my-app", tmplData.Environment.Values["appName"], + ".Environment.Values should contain env Values") + assert.Equal(t, "cliValue", tmplData.Environment.Values["cliFlag"], + ".Environment.Values should contain CLI overrides") +}