From 0139304d97ba700cf40d1ff776a6c8e19221205e Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 7 May 2026 15:50:05 +0200 Subject: [PATCH] feat(state): add mergeStrategy: fallback for first-file-wins env values (#2578) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(state): add mergeStrategy field to EnvironmentSpec Introduces a per-environment mergeStrategy with valid values "override" (default, current behavior) and "fallback". This commit only adds the field, the constants, and a parse-time validator; the loader still ignores the value, so behavior is unchanged. Subsequent commits thread the value through the values loader and implement the fallback semantics. Signed-off-by: Dominik Schmidt * refactor(state): thread mergeStrategy through values loader Adds a mergeStrategy string parameter to LoadEnvironmentValues, loadValuesEntries, and mapMerge so the value can flow from EnvironmentSpec down to the merge call site. Behavior is unchanged in this commit; mapMerge ignores the strategy and the next commit implements the fallback semantics. Top-level state.DefaultValues and the --state-values-file/-set loaders are passed an empty strategy ("") since they have no per-environment spec to consult and stay on the default override behavior. Signed-off-by: Dominik Schmidt * feat(state): implement fallback merge strategy Adds a hand-rolled fallbackDeepMerge that, unlike mergo, preserves keys present in the destination even when their value is the zero value (false, 0, "", nil, empty list/map). mapMerge dispatches to it when mergeStrategy == "fallback"; "override" and the empty default keep using mergo with WithOverride so existing behaviour is unchanged. Validation lives at the entry of LoadEnvironmentValues so a single chokepoint guards the field. Invalid values produce an error naming both the offending value and the valid options. Tests cover: first-file-wins precedence, gap filling, deep nested merge, three-file chains, explicit zero-value preservation (the case naïve mergo gets wrong), explicit nil preservation, inline map entries, override regression, default-equals-override equivalence, and invalid-strategy errors. Signed-off-by: Dominik Schmidt * feat(state): expose prior-file values in fallback template context Under mergeStrategy: fallback, .gotmpl values files can now reference values from earlier files in the same `values:` list via .Values (e.g. `service.domain: "service.{{ .Values.cluster.domain }}"`). The accumulated result is layered under env.GetMergedValues so env defaults, env values, and CLI overrides still win on overlap. Override mode keeps the historical template context — unchanged — so this is strictly opt-in via the mergeStrategy field. Together with the precedence flip from the previous commit, this lets users replace the brittle two-stage `merged-values.yaml.gotmpl` workaround with native helmfile syntax. Tests cover the headline cross-file template reference case and pin the override-mode contract that prior-file values stay invisible. Signed-off-by: Dominik Schmidt * docs: document mergeStrategy and fallback semantics Adds a new section to values-and-merging.md describing the override vs fallback strategies, the explicit-zero-value preservation guarantee, and the cross-file template reference behavior. Adds a brief pointer to environments.md so users land on the new field from the environment values discussion. Signed-off-by: Dominik Schmidt * refactor(state): reuse maputil.MergeMaps for fallback merge Replaces the hand-rolled fallbackDeepMerge with a single call to maputil.MergeMaps, swapping its arguments so the accumulated dest wins over the new src file. Same first-file-wins semantic, fewer lines, and the fallback path now inherits the same slice merge strategies the rest of helmfile already uses. The one observable behavior shift is for explicit nil values: under fallback, nil in an earlier file no longer 'wins' over a non-nil value in a later file — instead it falls through (matching MergeMaps' rule that nil from the override side only fills missing keys). This is internally consistent: nil-overwrites is an mergo.WithOverride quirk that lives only in the override path. The renamed test NilFallsThroughToFallback pins the new behavior with a comment referencing the contrast with override mode (Issue1154). Signed-off-by: Dominik Schmidt --------- Signed-off-by: Dominik Schmidt --- docs/environments.md | 15 + docs/values-and-merging.md | 48 +++ pkg/app/desired_state_file_loader.go | 4 +- pkg/state/create.go | 33 +- pkg/state/create_test.go | 93 ++++++ pkg/state/environment.go | 13 + pkg/state/envvals_loader.go | 89 +++-- pkg/state/envvals_loader_test.go | 306 +++++++++++++++++- pkg/state/state.go | 6 + pkg/state/testdata/mergestrategy/chain_a.yaml | 4 + pkg/state/testdata/mergestrategy/chain_b.yaml | 4 + pkg/state/testdata/mergestrategy/chain_c.yaml | 3 + pkg/state/testdata/mergestrategy/default.yaml | 2 + .../testdata/mergestrategy/fallback.yaml | 5 + .../mergestrategy/fallback.yaml.gotmpl | 3 + pkg/state/testdata/mergestrategy/glob_a.yaml | 2 + .../testdata/mergestrategy/glob_b.yaml.gotmpl | 2 + .../testdata/mergestrategy/nil_default.yaml | 1 + .../testdata/mergestrategy/nil_fallback.yaml | 1 + .../testdata/mergestrategy/zero_default.yaml | 4 + .../testdata/mergestrategy/zero_fallback.yaml | 6 + 21 files changed, 597 insertions(+), 47 deletions(-) create mode 100644 pkg/state/testdata/mergestrategy/chain_a.yaml create mode 100644 pkg/state/testdata/mergestrategy/chain_b.yaml create mode 100644 pkg/state/testdata/mergestrategy/chain_c.yaml create mode 100644 pkg/state/testdata/mergestrategy/default.yaml create mode 100644 pkg/state/testdata/mergestrategy/fallback.yaml create mode 100644 pkg/state/testdata/mergestrategy/fallback.yaml.gotmpl create mode 100644 pkg/state/testdata/mergestrategy/glob_a.yaml create mode 100644 pkg/state/testdata/mergestrategy/glob_b.yaml.gotmpl create mode 100644 pkg/state/testdata/mergestrategy/nil_default.yaml create mode 100644 pkg/state/testdata/mergestrategy/nil_fallback.yaml create mode 100644 pkg/state/testdata/mergestrategy/zero_default.yaml create mode 100644 pkg/state/testdata/mergestrategy/zero_fallback.yaml diff --git a/docs/environments.md b/docs/environments.md index 41eb7476..cdc7eee6 100644 --- a/docs/environments.md +++ b/docs/environments.md @@ -104,6 +104,21 @@ releases: ... ``` +#### Merge strategy + +By default, when several files are listed under `values:`, later files override earlier files. Set `mergeStrategy: fallback` on the environment to flip the precedence so earlier files win and later files only fill missing keys: + +```yaml +environments: + production: + mergeStrategy: fallback + values: + - cluster-specific.yaml # wins on every key it defines + - shared-defaults.yaml.gotmpl +``` + +Under `fallback`, explicit non-nil values in the earlier file (including zero values like `false`, `0`, `""`, and empty list) are preserved against any later file, while maps are deep-merged so later files may still add nested keys. A later `.gotmpl` file can also reference values from earlier files via `.Values`. See [Merge Strategy: override vs fallback](values-and-merging.md#4a-merge-strategy-override-vs-fallback) for the full semantics, including how explicit `null` is handled. + #### HCL specifications Since Helmfile v0.164.0, HCL language is supported for environment values only. diff --git a/docs/values-and-merging.md b/docs/values-and-merging.md index 5b39d80f..9237a8c3 100644 --- a/docs/values-and-merging.md +++ b/docs/values-and-merging.md @@ -116,6 +116,54 @@ environments: - secrets.yaml # Merged last (step 3) - highest priority ``` +### 4a. Merge Strategy: `override` vs `fallback` + +By default, when an environment lists multiple files under `values:`, **later files override earlier files** (the historical helmfile behavior, equivalent to `mergeStrategy: override`). + +You can flip this per environment so that **earlier files take precedence** and later files only fill in missing keys: + +```yaml +environments: + production: + mergeStrategy: fallback + values: + - cluster-specific.yaml # wins on every key it defines + - shared-defaults.yaml # only fills gaps +``` + +Under `fallback`: + +- An explicit non-nil value in an earlier file is preserved against any later file, including the zero values `false`, `0`, `""`, and empty list. An explicit `enabled: false` in `cluster-specific.yaml` is *not* silently overwritten by `enabled: true` from `shared-defaults.yaml`. +- Maps are deep-merged. An earlier map does not block later files from adding nested keys it didn't set; only the keys an earlier file explicitly defines win on conflict. +- An explicit `null` in an earlier file falls through to a later file's value, matching how `MergeMaps` treats nil from the override side elsewhere in helmfile. +- Within a single `values:` entry that expands to multiple files (e.g. via a glob), the **first** file in the expansion wins. +- A later `.gotmpl` values file can reference values from earlier files via `.Values`, so derived defaults work natively: + + ```yaml + # cluster-specific.yaml + cluster: + domain: prod.example.com + ``` + + ```yaml + # shared-defaults.yaml.gotmpl + service: + domain: "service.{{ .Values.cluster.domain }}" + ``` + + → `service.domain: service.prod.example.com`. Under `mergeStrategy: override` this cross-file template reference is not available. + +Valid values: `override` (default) and `fallback`. Any other value is rejected at load time. + +#### Interaction with `.hcl` values files + +`.hcl` files are evaluated as a single unit so that HCL `locals` and `values` blocks can reference each other across files. Helmfile collects every `.hcl` entry from the `values:` list, renders them together, and merges the combined result after the YAML pass. Two consequences worth knowing: + +- `mergeStrategy` does not reshuffle the position of HCL within the list. HCL's combined output is always merged after the YAML pass. Under `override` it overrides YAML on conflicts (the historical behavior); under `fallback` it fills gaps only. +- Among multiple `.hcl` files, the precedence is HCL's own (last-file-wins within HCL), independent of `mergeStrategy`. The strategy applies at the YAML-vs-HCL boundary, not within HCL. + +If you need first-file-wins precedence between specific HCL files, restructure them into one HCL file (or split the values into YAML). + ### 5. CLI Overrides The highest priority values come from CLI flags: diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index 8ffe3942..b57a029b 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -69,7 +69,7 @@ func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, e // --state-values-file: loaded into Values so arrays replace (not merge) if len(fileArgs) > 0 { - fileVals, err := envld.LoadEnvironmentValues(&handler, fileArgs, environment.New(ld.env), ld.env) + fileVals, err := envld.LoadEnvironmentValues(&handler, fileArgs, environment.New(ld.env), ld.env, "") if err != nil { return nil, err } @@ -78,7 +78,7 @@ func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, e // --state-values-set: loaded into CLIOverrides so arrays merge element-by-element if len(setArgs) > 0 { - setVals, err := envld.LoadEnvironmentValues(&handler, setArgs, environment.New(ld.env), ld.env) + setVals, err := envld.LoadEnvironmentValues(&handler, setArgs, environment.New(ld.env), ld.env, "") if err != nil { return nil, err } diff --git a/pkg/state/create.go b/pkg/state/create.go index 9058e9a7..7384596a 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -169,7 +169,7 @@ func (c *StateCreator) LoadEnvValues(target *HelmState, env string, failOnMissin return nil, &StateLoadError{fmt.Sprintf("failed to read %s", state.FilePath), err} } - newDefaults, err := state.loadValuesEntries(nil, state.DefaultValues, c.remote, ctxEnv, env) + newDefaults, err := state.loadValuesEntries(nil, state.DefaultValues, c.remote, ctxEnv, env, "") if err != nil { return nil, err } @@ -237,17 +237,12 @@ func mergeEnvironments(dst, src map[string]EnvironmentSpec) { for envName, srcEnv := range src { if dstEnv, exists := dst[envName]; exists { - // Environment exists in both - merge the Values slices - mergedValues := append([]any{}, dstEnv.Values...) - mergedValues = append(mergedValues, srcEnv.Values...) - // Merge Secrets slices mergedSecrets := append([]string{}, dstEnv.Secrets...) mergedSecrets = append(mergedSecrets, srcEnv.Secrets...) // Create merged environment merged := EnvironmentSpec{ - Values: mergedValues, Secrets: mergedSecrets, } @@ -272,6 +267,26 @@ func mergeEnvironments(dst, src map[string]EnvironmentSpec) { merged.MissingFileHandlerConfig = dstEnv.MissingFileHandlerConfig } + // Override MergeStrategy if src has it + if srcEnv.MergeStrategy != "" { + merged.MergeStrategy = srcEnv.MergeStrategy + } else { + merged.MergeStrategy = dstEnv.MergeStrategy + } + + // Concatenate Values so the later layer (src, e.g. main helmfile) + // always takes precedence over the earlier one (dst, e.g. a base + // helmfile). Under override the natural append order achieves that + // (last-file-wins). Under fallback we have to prepend src so that + // "first-file-wins" still resolves to "later layer wins". + if merged.MergeStrategy == MergeStrategyFallback { + mergedValues := append([]any{}, srcEnv.Values...) + merged.Values = append(mergedValues, dstEnv.Values...) + } else { + mergedValues := append([]any{}, dstEnv.Values...) + merged.Values = append(mergedValues, srcEnv.Values...) + } + dst[envName] = merged } else { // Environment only exists in src - just copy it @@ -394,7 +409,7 @@ func (c *StateCreator) loadEnvValues(st *HelmState, name string, failOnMissingEn if err != nil { return nil, err } - valuesVals, err = st.loadValuesEntries(envSpec.MissingFileHandler, envValuesEntries, c.remote, loadValuesEntriesEnv, name) + valuesVals, err = st.loadValuesEntries(envSpec.MissingFileHandler, envValuesEntries, c.remote, loadValuesEntriesEnv, name, envSpec.MergeStrategy) if err != nil { return nil, err } @@ -550,13 +565,13 @@ func (c *StateCreator) scatterGatherEnvSecretFiles(st *HelmState, envSecretFiles return decryptedFilesKeeper, nil } -func (st *HelmState) loadValuesEntries(missingFileHandler *string, entries []any, remote *remote.Remote, ctxEnv *environment.Environment, envName string) (map[string]any, error) { +func (st *HelmState) loadValuesEntries(missingFileHandler *string, entries []any, remote *remote.Remote, ctxEnv *environment.Environment, envName string, mergeStrategy string) (map[string]any, error) { var envVals map[string]any valuesEntries := append([]any{}, entries...) ld := NewEnvironmentValuesLoader(st.storage(), st.fs, st.logger, remote) var err error - envVals, err = ld.LoadEnvironmentValues(missingFileHandler, valuesEntries, ctxEnv, envName) + envVals, err = ld.LoadEnvironmentValues(missingFileHandler, valuesEntries, ctxEnv, envName, mergeStrategy) if err != nil { return nil, err } diff --git a/pkg/state/create_test.go b/pkg/state/create_test.go index aa6b704e..0d28753c 100644 --- a/pkg/state/create_test.go +++ b/pkg/state/create_test.go @@ -932,3 +932,96 @@ releases: }) } } + +// mergeEnvironments must keep the established "later layer wins over earlier +// layer" semantics even when the resolved strategy is fallback. Under override +// the natural append order (dst then src) achieves that because last-wins +// chooses src; under fallback we have to prepend src so that first-wins still +// resolves to src. Without this adjustment, a base helmfile's values would +// silently override values declared in the main helmfile that includes it. +func TestMergeEnvironments_LaterLayerWinsRegardlessOfStrategy(t *testing.T) { + tests := []struct { + name string + strategy string + wantOrdering []any + }{ + { + name: "override appends src after dst (last wins)", + strategy: MergeStrategyOverride, + wantOrdering: []any{"base.yaml", "main.yaml"}, + }, + { + name: "empty strategy defaults to override semantics", + strategy: "", + wantOrdering: []any{"base.yaml", "main.yaml"}, + }, + { + name: "fallback prepends src (first wins) so main still beats base", + strategy: MergeStrategyFallback, + wantOrdering: []any{"main.yaml", "base.yaml"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dst := map[string]EnvironmentSpec{ + "prod": {Values: []any{"base.yaml"}, MergeStrategy: tt.strategy}, + } + src := map[string]EnvironmentSpec{ + "prod": {Values: []any{"main.yaml"}, MergeStrategy: tt.strategy}, + } + mergeEnvironments(dst, src) + got := dst["prod"].Values + if !reflect.DeepEqual(got, tt.wantOrdering) { + t.Errorf("Values ordering: want %v, got %v", tt.wantOrdering, got) + } + }) + } +} + +// mergeEnvironments must preserve MergeStrategy when layering multiple +// helmfiles (bases). Without this, an environment that opted into +// fallback in a base helmfile would silently fall back to the default +// override behavior in any helmfile that re-declares the environment. +func TestMergeEnvironments_PreservesMergeStrategy(t *testing.T) { + tests := []struct { + name string + dst EnvironmentSpec + src EnvironmentSpec + expected string + }{ + { + name: "src declares fallback, dst empty", + dst: EnvironmentSpec{}, + src: EnvironmentSpec{MergeStrategy: MergeStrategyFallback}, + expected: MergeStrategyFallback, + }, + { + name: "dst declares fallback, src empty preserves dst", + dst: EnvironmentSpec{MergeStrategy: MergeStrategyFallback}, + src: EnvironmentSpec{}, + expected: MergeStrategyFallback, + }, + { + name: "src override wins over dst fallback", + dst: EnvironmentSpec{MergeStrategy: MergeStrategyFallback}, + src: EnvironmentSpec{MergeStrategy: MergeStrategyOverride}, + expected: MergeStrategyOverride, + }, + { + name: "both empty stays empty", + dst: EnvironmentSpec{}, + src: EnvironmentSpec{}, + expected: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dst := map[string]EnvironmentSpec{"prod": tt.dst} + src := map[string]EnvironmentSpec{"prod": tt.src} + mergeEnvironments(dst, src) + if got := dst["prod"].MergeStrategy; got != tt.expected { + t.Errorf("MergeStrategy after merge: want %q, got %q", tt.expected, got) + } + }) + } +} diff --git a/pkg/state/environment.go b/pkg/state/environment.go index c5c78876..91d997ab 100644 --- a/pkg/state/environment.go +++ b/pkg/state/environment.go @@ -15,4 +15,17 @@ type EnvironmentSpec struct { MissingFileHandler *string `yaml:"missingFileHandler,omitempty"` // MissingFileHandlerConfig is composed of various settings for the MissingFileHandler MissingFileHandlerConfig *MissingFileHandlerConfig `yaml:"missingFileHandlerConfig,omitempty"` + + // MergeStrategy controls precedence when multiple values files are listed under `values`. + // + // "override" (default): later files override earlier files (the historical helmfile behavior). + // "fallback": earlier files take precedence; later files only fill gaps. + // + // Under the "fallback" strategy, an explicit non-nil value in an earlier file (including + // the zero values false, 0, "", and empty list) is preserved against any later file. Maps + // are deep-merged, so an earlier map does not block later files from adding nested keys. + // An explicit null in an earlier file falls through to a later file's value (matching how + // helmfile's MergeMaps treats nil from the override side elsewhere). Subsequent .gotmpl + // values files can also reference values from earlier files via .Values. + MergeStrategy string `yaml:"mergeStrategy,omitempty"` } diff --git a/pkg/state/envvals_loader.go b/pkg/state/envvals_loader.go index 8764025b..048154a8 100644 --- a/pkg/state/envvals_loader.go +++ b/pkg/state/envvals_loader.go @@ -36,7 +36,13 @@ func NewEnvironmentValuesLoader(storage *Storage, fs *filesystem.FileSystem, log } } -func (ld *EnvironmentValuesLoader) LoadEnvironmentValues(missingFileHandler *string, valuesEntries []any, ctxEnv *environment.Environment, envName string) (map[string]any, error) { +func (ld *EnvironmentValuesLoader) LoadEnvironmentValues(missingFileHandler *string, valuesEntries []any, ctxEnv *environment.Environment, envName string, mergeStrategy string) (map[string]any, error) { + switch mergeStrategy { + case "", MergeStrategyOverride, MergeStrategyFallback: + default: + return nil, fmt.Errorf("environment %q: invalid mergeStrategy %q (must be %q or %q)", + envName, mergeStrategy, MergeStrategyOverride, MergeStrategyFallback) + } var ( result = map[string]any{} hclLoader = hcllang.NewHCLLoader(ld.fs, ld.logger) @@ -44,8 +50,6 @@ func (ld *EnvironmentValuesLoader) LoadEnvironmentValues(missingFileHandler *str ) for _, entry := range valuesEntries { - maps := []any{} - switch strOrMap := entry.(type) { case string: files, skipped, err := ld.storage.resolveFile(missingFileHandler, "environment values", entry.(string)) @@ -64,37 +68,56 @@ func (ld *EnvironmentValuesLoader) LoadEnvironmentValues(missingFileHandler *str } if strings.HasSuffix(f, ".hcl") { hclLoader.AddFile(f) - } else { - // Use merged values (Defaults + Values + CLIOverrides) for template rendering - // so that CLI values are accessible via .Values in environment value files. - mergedVals, err := env.GetMergedValues() - if err != nil { - return nil, fmt.Errorf("failed to get merged values for environment file \"%s\": %v", f, err) + continue + } + // Use merged values (Defaults + Values + CLIOverrides) for template rendering + // so that CLI values are accessible via .Values in environment value files. + mergedVals, err := env.GetMergedValues() + if err != nil { + return nil, fmt.Errorf("failed to get merged values for environment file \"%s\": %v", f, err) + } + // Under fallback strategy, also expose values accumulated from earlier files + // in this same `values:` list, including earlier files in this same glob + // expansion, so a later .gotmpl can reference them via .Values (e.g. + // `{{ .Values.cluster.domain }}`). Env CLI overrides and values still win, + // layered on top with WithOverride. + if mergeStrategy == MergeStrategyFallback && len(result) > 0 { + enriched := map[string]any{} + if err := mergo.Merge(&enriched, result); err != nil { + return nil, fmt.Errorf("failed to build template context for \"%s\": %v", f, err) } - tmplData := NewEnvironmentTemplateData(env, "", mergedVals) - r := tmpl.NewFileRenderer(ld.fs, filepath.Dir(f), tmplData) - bytes, err := r.RenderToBytes(f) - if err != nil { - return nil, fmt.Errorf("failed to load environment values file \"%s\": %v", f, err) + if err := mergo.Merge(&enriched, mergedVals, mergo.WithOverride); err != nil { + return nil, fmt.Errorf("failed to build template context for \"%s\": %v", f, err) } - m := map[string]any{} - if err := yaml.Unmarshal(bytes, &m); err != nil { - return nil, fmt.Errorf("failed to load environment values file \"%s\": %v\n\nOffending YAML:\n%s", f, err, bytes) - } - maps = append(maps, m) - ld.logger.Debugf("envvals_loader: loaded %s:%v", strOrMap, m) + mergedVals = enriched + } + tmplData := NewEnvironmentTemplateData(env, "", mergedVals) + r := tmpl.NewFileRenderer(ld.fs, filepath.Dir(f), tmplData) + bytes, err := r.RenderToBytes(f) + if err != nil { + return nil, fmt.Errorf("failed to load environment values file \"%s\": %v", f, err) + } + m := map[string]any{} + if err := yaml.Unmarshal(bytes, &m); err != nil { + return nil, fmt.Errorf("failed to load environment values file \"%s\": %v\n\nOffending YAML:\n%s", f, err, bytes) + } + ld.logger.Debugf("envvals_loader: loaded %s:%v", strOrMap, m) + // Merge each file into result immediately so subsequent files in the same + // entry's expansion (e.g. a glob) can see prior files' values via .Values + // when rendered as templates. + result, err = mapMerge(result, []any{m}, mergeStrategy) + if err != nil { + return nil, err } } case map[any]any, map[string]any: - maps = append(maps, strOrMap) + result, err = mapMerge(result, []any{strOrMap}, mergeStrategy) + if err != nil { + return nil, err + } default: return nil, fmt.Errorf("unexpected type of value: value=%v, type=%T", strOrMap, strOrMap) } - - result, err = mapMerge(result, maps) - if err != nil { - return nil, err - } } maps := []any{} if hclLoader.Length() > 0 { @@ -104,14 +127,14 @@ func (ld *EnvironmentValuesLoader) LoadEnvironmentValues(missingFileHandler *str } maps = append(maps, m) } - result, err = mapMerge(result, maps) + result, err = mapMerge(result, maps, mergeStrategy) if err != nil { return nil, err } return result, nil } -func mapMerge(dest map[string]any, maps []any) (map[string]any, error) { +func mapMerge(dest map[string]any, maps []any, mergeStrategy string) (map[string]any, error) { for _, m := range maps { // All the nested map key should be string. Otherwise we get strange errors due to that // mergo or reflect is unable to merge map[any]any with map[string]any or vice versa. @@ -120,6 +143,16 @@ func mapMerge(dest map[string]any, maps []any) (map[string]any, error) { if err != nil { return nil, err } + if mergeStrategy == MergeStrategyFallback { + // First-file-wins: the new file is the base and the + // accumulator overlays it, so keys already accumulated keep + // their value while keys only present in the new file fill + // in. MergeMaps is used instead of mergo because mergo's + // isEmptyValue rule would silently let a later fallback's + // `enabled: true` clobber an explicit `enabled: false`. + dest = maputil.MergeMaps(vals, dest) + continue + } if err := mergo.Merge(&dest, &vals, mergo.WithOverride); err != nil { return nil, fmt.Errorf("failed to merge %v: %v", m, err) } diff --git a/pkg/state/envvals_loader_test.go b/pkg/state/envvals_loader_test.go index 764ada78..5902ebe6 100644 --- a/pkg/state/envvals_loader_test.go +++ b/pkg/state/envvals_loader_test.go @@ -2,6 +2,7 @@ package state import ( "io" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -29,7 +30,7 @@ func newLoader() *EnvironmentValuesLoader { func TestEnvValsLoad_SingleValuesFile(t *testing.T) { l := newLoader() - actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.5.yaml"}, nil, "") + actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.5.yaml"}, nil, "", "") if err != nil { t.Fatal(err) } @@ -87,7 +88,7 @@ func TestEnvValsLoad_EnvironmentNameFile(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.6.yaml.gotmpl"}, tt.env, tt.envName) + actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.6.yaml.gotmpl"}, tt.env, tt.envName, "") if err != nil { t.Fatal(err) } @@ -103,7 +104,7 @@ func TestEnvValsLoad_EnvironmentNameFile(t *testing.T) { func TestEnvValsLoad_SingleValuesFileRemote(t *testing.T) { l := newLoader() - actual, err := l.LoadEnvironmentValues(nil, []any{"git::https://github.com/helm/helm.git@cmd/helm/testdata/output/values.yaml?ref=v3.8.1"}, nil, "") + actual, err := l.LoadEnvironmentValues(nil, []any{"git::https://github.com/helm/helm.git@cmd/helm/testdata/output/values.yaml?ref=v3.8.1"}, nil, "", "") if err != nil { t.Fatal(err) } @@ -121,7 +122,7 @@ func TestEnvValsLoad_SingleValuesFileRemote(t *testing.T) { func TestEnvValsLoad_OverwriteNilValue_Issue1150(t *testing.T) { l := newLoader() - actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.1.yaml", "testdata/values.2.yaml"}, nil, "") + actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.1.yaml", "testdata/values.2.yaml"}, nil, "", "") if err != nil { t.Fatal(err) } @@ -143,7 +144,7 @@ func TestEnvValsLoad_OverwriteNilValue_Issue1150(t *testing.T) { func TestEnvValsLoad_OverwriteWithNilValue_Issue1154(t *testing.T) { l := newLoader() - actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.3.yaml", "testdata/values.4.yaml"}, nil, "") + actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.3.yaml", "testdata/values.4.yaml"}, nil, "", "") if err != nil { t.Fatal(err) } @@ -166,7 +167,7 @@ func TestEnvValsLoad_OverwriteWithNilValue_Issue1154(t *testing.T) { func TestEnvValsLoad_OverwriteEmptyValue_Issue1168(t *testing.T) { l := newLoader() - actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/issues/1168/addons.yaml", "testdata/issues/1168/addons2.yaml"}, nil, "") + actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/issues/1168/addons.yaml", "testdata/issues/1168/addons2.yaml"}, nil, "", "") if err != nil { t.Fatal(err) } @@ -191,7 +192,7 @@ func TestEnvValsLoad_OverwriteEmptyValue_Issue1168(t *testing.T) { func TestEnvValsLoad_MultiHCL(t *testing.T) { l := newLoader() - actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.7.hcl", "testdata/values.8.hcl"}, nil, "") + actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.7.hcl", "testdata/values.8.hcl"}, nil, "", "") if err != nil { t.Fatal(err) } @@ -234,7 +235,7 @@ func TestEnvValsLoad_EnvironmentValues(t *testing.T) { env := environment.New("test") env.Values["foo"] = "bar" - actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.9.yaml.gotmpl"}, env, "") + actual, err := l.LoadEnvironmentValues(nil, []any{"testdata/values.9.yaml.gotmpl"}, env, "", "") if err != nil { t.Fatal(err) } @@ -247,3 +248,292 @@ func TestEnvValsLoad_EnvironmentValues(t *testing.T) { t.Error(diff) } } + +// --- mergeStrategy: fallback --- + +// Earlier files take precedence. Same conflicting key in two files → +// the value from default.yaml (loaded first) wins. +func TestEnvValsLoad_FallbackStrategy_EarlierWins(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml", "testdata/mergestrategy/fallback.yaml"}, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + cluster := actual["cluster"].(map[string]any) + if got := cluster["domain"]; got != "example.com" { + t.Errorf("cluster.domain: want %q (from default.yaml), got %v", "example.com", got) + } +} + +// Later files only fill keys that are missing from earlier files. +func TestEnvValsLoad_FallbackStrategy_FillsGaps(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml", "testdata/mergestrategy/fallback.yaml"}, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + cluster := actual["cluster"].(map[string]any) + if got := cluster["region"]; got != "us-east-1" { + t.Errorf("cluster.region: want %q (from fallback.yaml, missing in default.yaml), got %v", "us-east-1", got) + } + service := actual["service"].(map[string]any) + if got := service["port"]; got != 8080 { + t.Errorf("service.port: want 8080 (from fallback.yaml), got %v", got) + } +} + +// Nested maps merge recursively: top-level cluster is not replaced +// wholesale; both files contribute keys. +func TestEnvValsLoad_FallbackStrategy_DeepMerge(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml", "testdata/mergestrategy/fallback.yaml"}, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + expected := map[string]any{ + "cluster": map[string]any{ + "domain": "example.com", // from default (wins) + "region": "us-east-1", // from fallback (gap filled) + }, + "service": map[string]any{ + "port": 8080, // from fallback (gap filled) + }, + } + if diff := cmp.Diff(expected, actual); diff != "" { + t.Errorf("deep merge mismatch (-want +got):\n%s", diff) + } +} + +// First-wins precedence holds across an arbitrarily long chain, not just +// pairwise. Three files exercise the accumulator state across iterations. +func TestEnvValsLoad_FallbackStrategy_ChainedFiles(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{ + "testdata/mergestrategy/chain_a.yaml", + "testdata/mergestrategy/chain_b.yaml", + "testdata/mergestrategy/chain_c.yaml", + }, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + expected := map[string]any{ + "letter": "a", // only in a + "only_a": "from-a", // only in a + "only_b": "from-b", // only in b + "only_c": "from-c", // only in c + "both_ab": "from-a", // a and b → a wins (earlier) + "both_bc": "from-b", // b and c → b wins (earlier) + "all_three": "from-a", // a, b, c → a wins (earliest) + } + if diff := cmp.Diff(expected, actual); diff != "" { + t.Errorf("chain mismatch (-want +got):\n%s", diff) + } +} + +// Explicit zero values in the earlier file MUST be preserved. Without the +// hand-rolled fallbackDeepMerge, mergo's isEmptyValue would silently let +// `enabled: true` from fallback overwrite `enabled: false` from default. +func TestEnvValsLoad_FallbackStrategy_PreservesExplicitZeroValues(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/zero_default.yaml", "testdata/mergestrategy/zero_fallback.yaml"}, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + expected := map[string]any{ + "enabled": false, + "replicas": 0, + "name": "", + "tags": []any{}, + } + if diff := cmp.Diff(expected, actual); diff != "" { + t.Errorf("explicit zero values not preserved (-want +got):\n%s", diff) + } +} + +// Explicit nil in the earlier file does NOT win under fallback: it falls +// through to the fallback file's value. This matches helmfile's existing +// MergeMaps treatment of nil ("nil from the override side only fills missing +// keys"; here, by argument-swap, nil from the winner is treated as +// "no preference, let the fallback fill it"). Documented as the deliberate +// difference from override mode, where mergo.WithOverride lets nil overwrite +// (see TestEnvValsLoad_OverwriteWithNilValue_Issue1154). +func TestEnvValsLoad_FallbackStrategy_NilFallsThroughToFallback(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/nil_default.yaml", "testdata/mergestrategy/nil_fallback.yaml"}, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + expected := map[string]any{"value": "from-fallback"} + if diff := cmp.Diff(expected, actual); diff != "" { + t.Errorf("explicit nil should fall through to fallback (-want +got):\n%s", diff) + } +} + +// Inline map entries (not file paths) also honor the fallback strategy. +func TestEnvValsLoad_FallbackStrategy_InlineMapEntry(t *testing.T) { + l := newLoader() + + inline := map[string]any{ + "cluster": map[string]any{"domain": "inline.example"}, + "extra": "from-inline", + } + + actual, err := l.LoadEnvironmentValues(nil, + []any{inline, "testdata/mergestrategy/fallback.yaml"}, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + cluster := actual["cluster"].(map[string]any) + if got := cluster["domain"]; got != "inline.example" { + t.Errorf("cluster.domain: want inline value to win, got %v", got) + } + if got := actual["extra"]; got != "from-inline" { + t.Errorf("extra: want %q, got %v", "from-inline", got) + } + // fallback.yaml still fills gaps the inline map did not set. + if got := cluster["region"]; got != "us-east-1" { + t.Errorf("cluster.region: want %q from fallback file, got %v", "us-east-1", got) + } +} + +// Regression guard: explicit "override" matches today's behavior +// (last file wins). +func TestEnvValsLoad_OverrideStrategy_PreservesCurrentBehavior(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml", "testdata/mergestrategy/fallback.yaml"}, + nil, "", MergeStrategyOverride) + if err != nil { + t.Fatal(err) + } + + cluster := actual["cluster"].(map[string]any) + if got := cluster["domain"]; got != "cluster.local" { + t.Errorf("cluster.domain under override: want %q (from fallback.yaml), got %v", "cluster.local", got) + } +} + +// Empty strategy is identical to explicit "override". +func TestEnvValsLoad_DefaultStrategy_MatchesOverride(t *testing.T) { + l := newLoader() + + asDefault, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml", "testdata/mergestrategy/fallback.yaml"}, + nil, "", "") + if err != nil { + t.Fatal(err) + } + asOverride, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml", "testdata/mergestrategy/fallback.yaml"}, + nil, "", MergeStrategyOverride) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(asOverride, asDefault); diff != "" { + t.Errorf("default strategy diverges from override (-override +default):\n%s", diff) + } +} + +// Within a single `values:` entry that expands to multiple files (a glob), a +// later .gotmpl in the expansion can reference earlier files in that same +// expansion. Matters because the inner file loop must merge each parsed file +// into the accumulator before rendering the next, not buffer the whole +// expansion and merge once at the end. +func TestEnvValsLoad_FallbackStrategy_GlobTemplateSeesPriorFileInSameExpansion(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/glob_*.yaml*"}, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + service := actual["service"].(map[string]any) + if got := service["domain"]; got != "service.example.com" { + t.Errorf("service.domain: want %q (templated from sibling glob match), got %v", + "service.example.com", got) + } +} + +// The headline use case: under fallback, a later .gotmpl values file can +// reference values defined by earlier files in the same list via .Values. +// default.yaml sets cluster.domain; fallback.yaml.gotmpl renders +// `service.domain: "service.{{ .Values.cluster.domain }}"`. +func TestEnvValsLoad_FallbackStrategy_TemplateAccessesPriorFile(t *testing.T) { + l := newLoader() + + actual, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml", "testdata/mergestrategy/fallback.yaml.gotmpl"}, + nil, "", MergeStrategyFallback) + if err != nil { + t.Fatal(err) + } + + service := actual["service"].(map[string]any) + if got := service["domain"]; got != "service.example.com" { + t.Errorf("service.domain: want %q (templated from prior file), got %v", + "service.example.com", got) + } +} + +// Symmetric guard: under override, the same .gotmpl reference does NOT +// see prior files in the same list. Documents the deliberate scoping: +// the cross-file template enrichment is opt-in via mergeStrategy: fallback. +func TestEnvValsLoad_OverrideStrategy_TemplateContextUnchanged(t *testing.T) { + l := newLoader() + + _, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml", "testdata/mergestrategy/fallback.yaml.gotmpl"}, + nil, "", MergeStrategyOverride) + if err == nil { + t.Fatal("expected template render error: under override, .Values.cluster.domain should not resolve to a prior file's value") + } + // The exact error wording is owned by the template renderer; we only + // assert that we got an error rather than a bogus successful render. +} + +// Unknown strategy values produce a clear error that names both the bad +// value and the valid options. +func TestEnvValsLoad_InvalidStrategy_Errors(t *testing.T) { + l := newLoader() + + _, err := l.LoadEnvironmentValues(nil, + []any{"testdata/mergestrategy/default.yaml"}, + nil, "prod", "bogus") + if err == nil { + t.Fatal("expected error for invalid mergeStrategy, got nil") + } + for _, want := range []string{"prod", "bogus", MergeStrategyOverride, MergeStrategyFallback} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error message missing %q: %v", want, err) + } + } +} diff --git a/pkg/state/state.go b/pkg/state/state.go index cec8f890..8a82e85e 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -49,6 +49,12 @@ const ( // Valid enum for updateStrategy values UpdateStrategyReinstallIfForbidden = "reinstallIfForbidden" + + // Valid values for environment mergeStrategy. + // MergeStrategyOverride (default) makes later values files override earlier ones. + // MergeStrategyFallback flips the precedence: earlier files win and later files only fill gaps. + MergeStrategyOverride = "override" + MergeStrategyFallback = "fallback" ) // ReleaseSetSpec is release set spec diff --git a/pkg/state/testdata/mergestrategy/chain_a.yaml b/pkg/state/testdata/mergestrategy/chain_a.yaml new file mode 100644 index 00000000..4c23a0f4 --- /dev/null +++ b/pkg/state/testdata/mergestrategy/chain_a.yaml @@ -0,0 +1,4 @@ +letter: a +only_a: from-a +both_ab: from-a +all_three: from-a diff --git a/pkg/state/testdata/mergestrategy/chain_b.yaml b/pkg/state/testdata/mergestrategy/chain_b.yaml new file mode 100644 index 00000000..19d208f6 --- /dev/null +++ b/pkg/state/testdata/mergestrategy/chain_b.yaml @@ -0,0 +1,4 @@ +only_b: from-b +both_ab: from-b +both_bc: from-b +all_three: from-b diff --git a/pkg/state/testdata/mergestrategy/chain_c.yaml b/pkg/state/testdata/mergestrategy/chain_c.yaml new file mode 100644 index 00000000..2fd3686f --- /dev/null +++ b/pkg/state/testdata/mergestrategy/chain_c.yaml @@ -0,0 +1,3 @@ +only_c: from-c +both_bc: from-c +all_three: from-c diff --git a/pkg/state/testdata/mergestrategy/default.yaml b/pkg/state/testdata/mergestrategy/default.yaml new file mode 100644 index 00000000..4ad18014 --- /dev/null +++ b/pkg/state/testdata/mergestrategy/default.yaml @@ -0,0 +1,2 @@ +cluster: + domain: example.com diff --git a/pkg/state/testdata/mergestrategy/fallback.yaml b/pkg/state/testdata/mergestrategy/fallback.yaml new file mode 100644 index 00000000..8b57825e --- /dev/null +++ b/pkg/state/testdata/mergestrategy/fallback.yaml @@ -0,0 +1,5 @@ +cluster: + domain: cluster.local + region: us-east-1 +service: + port: 8080 diff --git a/pkg/state/testdata/mergestrategy/fallback.yaml.gotmpl b/pkg/state/testdata/mergestrategy/fallback.yaml.gotmpl new file mode 100644 index 00000000..0df72cba --- /dev/null +++ b/pkg/state/testdata/mergestrategy/fallback.yaml.gotmpl @@ -0,0 +1,3 @@ +service: + domain: "service.{{ .Values.cluster.domain }}" + port: 8080 diff --git a/pkg/state/testdata/mergestrategy/glob_a.yaml b/pkg/state/testdata/mergestrategy/glob_a.yaml new file mode 100644 index 00000000..4ad18014 --- /dev/null +++ b/pkg/state/testdata/mergestrategy/glob_a.yaml @@ -0,0 +1,2 @@ +cluster: + domain: example.com diff --git a/pkg/state/testdata/mergestrategy/glob_b.yaml.gotmpl b/pkg/state/testdata/mergestrategy/glob_b.yaml.gotmpl new file mode 100644 index 00000000..2d92987e --- /dev/null +++ b/pkg/state/testdata/mergestrategy/glob_b.yaml.gotmpl @@ -0,0 +1,2 @@ +service: + domain: "service.{{ .Values.cluster.domain }}" diff --git a/pkg/state/testdata/mergestrategy/nil_default.yaml b/pkg/state/testdata/mergestrategy/nil_default.yaml new file mode 100644 index 00000000..6a1a5a05 --- /dev/null +++ b/pkg/state/testdata/mergestrategy/nil_default.yaml @@ -0,0 +1 @@ +value: ~ diff --git a/pkg/state/testdata/mergestrategy/nil_fallback.yaml b/pkg/state/testdata/mergestrategy/nil_fallback.yaml new file mode 100644 index 00000000..5a7504c9 --- /dev/null +++ b/pkg/state/testdata/mergestrategy/nil_fallback.yaml @@ -0,0 +1 @@ +value: from-fallback diff --git a/pkg/state/testdata/mergestrategy/zero_default.yaml b/pkg/state/testdata/mergestrategy/zero_default.yaml new file mode 100644 index 00000000..d1fb7157 --- /dev/null +++ b/pkg/state/testdata/mergestrategy/zero_default.yaml @@ -0,0 +1,4 @@ +enabled: false +replicas: 0 +name: "" +tags: [] diff --git a/pkg/state/testdata/mergestrategy/zero_fallback.yaml b/pkg/state/testdata/mergestrategy/zero_fallback.yaml new file mode 100644 index 00000000..fc39f730 --- /dev/null +++ b/pkg/state/testdata/mergestrategy/zero_fallback.yaml @@ -0,0 +1,6 @@ +enabled: true +replicas: 3 +name: from-fallback +tags: + - a + - b