* 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 <yxx@users.noreply.github.com> Signed-off-by: yxxhero <aiopsclub@163.com> * 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 <yxx@users.noreply.github.com> Signed-off-by: yxxhero <aiopsclub@163.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
This commit is contained in:
parent
fc6cf5d2cc
commit
b9378b17f6
|
|
@ -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"])
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
Loading…
Reference in New Issue