diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index 5b65d9a6..40452bec 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -65,8 +65,8 @@ func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, e } overrodeEnv = &environment.Environment{ - Name: ld.env, - Values: vals, + Name: ld.env, + CLIOverrides: vals, } } diff --git a/pkg/environment/environment.go b/pkg/environment/environment.go index 66eda04c..1926b6c9 100644 --- a/pkg/environment/environment.go +++ b/pkg/environment/environment.go @@ -1,17 +1,16 @@ package environment import ( - "dario.cat/mergo" - "github.com/helmfile/helmfile/pkg/maputil" "github.com/helmfile/helmfile/pkg/yaml" ) type Environment struct { - Name string - KubeContext string - Values map[string]any - Defaults map[string]any + Name string + KubeContext string + Values map[string]any + Defaults map[string]any + CLIOverrides map[string]any // CLI --state-values-set values, merged element-by-element } var EmptyEnvironment Environment @@ -19,10 +18,11 @@ var EmptyEnvironment Environment // New return Environment with default name and values func New(name string) *Environment { return &Environment{ - Name: name, - KubeContext: "", - Values: map[string]any{}, - Defaults: map[string]any{}, + Name: name, + KubeContext: "", + Values: map[string]any{}, + Defaults: map[string]any{}, + CLIOverrides: map[string]any{}, } } @@ -53,11 +53,25 @@ func (e Environment) DeepCopy() Environment { panic(err) } + cliOverridesBytes, err := yaml.Marshal(e.CLIOverrides) + if err != nil { + panic(err) + } + var cliOverrides map[string]any + if err := yaml.Unmarshal(cliOverridesBytes, &cliOverrides); err != nil { + panic(err) + } + cliOverrides, err = maputil.CastKeysToStrings(cliOverrides) + if err != nil { + panic(err) + } + return Environment{ - Name: e.Name, - KubeContext: e.KubeContext, - Values: values, - Defaults: defaults, + Name: e.Name, + KubeContext: e.KubeContext, + Values: values, + Defaults: defaults, + CLIOverrides: cliOverrides, } } @@ -71,9 +85,19 @@ func (e *Environment) Merge(other *Environment) (*Environment, error) { } copy := e.DeepCopy() if other != nil { - if err := mergo.Merge(©, other, mergo.WithOverride); err != nil { - return nil, err + // Merge scalar fields + if other.Name != "" { + copy.Name = other.Name } + if other.KubeContext != "" { + copy.KubeContext = other.KubeContext + } + // Merge Values - layer values replace arrays + copy.Values = maputil.MergeMaps(copy.Values, other.Values) + copy.Defaults = maputil.MergeMaps(copy.Defaults, other.Defaults) + // Merge CLIOverrides using element-by-element array merging + copy.CLIOverrides = maputil.MergeMaps(copy.CLIOverrides, other.CLIOverrides, + maputil.MergeOptions{ArrayStrategy: maputil.ArrayMergeStrategyMerge}) } return ©, nil } @@ -81,9 +105,11 @@ func (e *Environment) Merge(other *Environment) (*Environment, error) { func (e *Environment) GetMergedValues() (map[string]any, error) { vals := map[string]any{} 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) + // CLI overrides are merged last using element-by-element array merging. + // This ensures --state-values-set array[0]=x only changes that index. + vals = maputil.MergeMaps(vals, e.CLIOverrides, + maputil.MergeOptions{ArrayStrategy: maputil.ArrayMergeStrategyMerge}) vals, err := maputil.CastKeysToStrings(vals) if err != nil { diff --git a/pkg/maputil/maputil.go b/pkg/maputil/maputil.go index 099d6baf..f6c697f3 100644 --- a/pkg/maputil/maputil.go +++ b/pkg/maputil/maputil.go @@ -237,6 +237,11 @@ func MergeMaps(a, b map[string]interface{}, opts ...MergeOptions) map[string]int } for k, v := range b { if v == nil { + // If key doesn't exist in base, add nil (issue #1154). + // If key exists in base, don't overwrite with nil. + if _, exists := out[k]; !exists { + out[k] = nil + } continue } if v, ok := v.(map[string]interface{}); ok { diff --git a/pkg/state/create.go b/pkg/state/create.go index dfb20206..b212ceb8 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -397,24 +397,28 @@ func (c *StateCreator) loadEnvValues(st *HelmState, name string, failOnMissingEn return nil, &UndefinedEnvError{Env: name} } - newEnv := &environment.Environment{Name: name, Values: valuesVals, KubeContext: envSpec.KubeContext} + newEnv := &environment.Environment{Name: name, Values: valuesVals, KubeContext: envSpec.KubeContext, CLIOverrides: map[string]any{}} if ctxEnv != nil { - intCtxEnv := *ctxEnv - - if err := mergo.Merge(&intCtxEnv, newEnv, mergo.WithOverride); err != nil { - return nil, fmt.Errorf("error while merging environment values for \"%s\": %v", name, err) + // Merge layer values (arrays replace) + newEnv.Values = maputil.MergeMaps(newEnv.Values, ctxEnv.Values) + // Copy CLI overrides to be merged at GetMergedValues time + newEnv.CLIOverrides = maputil.MergeMaps(newEnv.CLIOverrides, ctxEnv.CLIOverrides, + maputil.MergeOptions{ArrayStrategy: maputil.ArrayMergeStrategyMerge}) + if ctxEnv.Name != "" { + newEnv.Name = ctxEnv.Name + } + if ctxEnv.KubeContext != "" { + newEnv.KubeContext = ctxEnv.KubeContext } - - newEnv = &intCtxEnv } if overrode != nil { - intOverrodeEnv := *newEnv - // CLI overrides always merge arrays element-by-element (ArrayMergeStrategyMerge) - intOverrodeEnv.Values = maputil.MergeMaps(intOverrodeEnv.Values, overrode.Values, + // Merge layer values from overrode (arrays replace) + newEnv.Values = maputil.MergeMaps(newEnv.Values, overrode.Values) + // Merge CLI overrides (arrays merge element-by-element) + newEnv.CLIOverrides = maputil.MergeMaps(newEnv.CLIOverrides, overrode.CLIOverrides, maputil.MergeOptions{ArrayStrategy: maputil.ArrayMergeStrategyMerge}) - newEnv = &intOverrodeEnv } return newEnv, nil