Files
helmfile/pkg/state/envvals_loader.go
T
Aditya Menon 70645e0622 fix: array merge regression - layer arrays now replace defaults (#2367)
* 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>
2026-01-18 14:04:54 +08:00

129 lines
3.6 KiB
Go

package state
import (
"fmt"
"path/filepath"
"strings"
"dario.cat/mergo"
"go.uber.org/zap"
"github.com/helmfile/helmfile/pkg/environment"
"github.com/helmfile/helmfile/pkg/filesystem"
"github.com/helmfile/helmfile/pkg/hcllang"
"github.com/helmfile/helmfile/pkg/maputil"
"github.com/helmfile/helmfile/pkg/remote"
"github.com/helmfile/helmfile/pkg/tmpl"
"github.com/helmfile/helmfile/pkg/yaml"
)
type EnvironmentValuesLoader struct {
storage *Storage
fs *filesystem.FileSystem
logger *zap.SugaredLogger
remote *remote.Remote
}
func NewEnvironmentValuesLoader(storage *Storage, fs *filesystem.FileSystem, logger *zap.SugaredLogger, remote *remote.Remote) *EnvironmentValuesLoader {
return &EnvironmentValuesLoader{
storage: storage,
fs: fs,
logger: logger,
remote: remote,
}
}
func (ld *EnvironmentValuesLoader) LoadEnvironmentValues(missingFileHandler *string, valuesEntries []any, ctxEnv *environment.Environment, envName string) (map[string]any, error) {
var (
result = map[string]any{}
hclLoader = hcllang.NewHCLLoader(ld.fs, ld.logger)
err error
)
for _, entry := range valuesEntries {
maps := []any{}
switch strOrMap := entry.(type) {
case string:
files, skipped, err := ld.storage.resolveFile(missingFileHandler, "environment values", entry.(string))
if err != nil {
return nil, err
}
if skipped {
continue
}
for _, f := range files {
var env environment.Environment
if ctxEnv == nil {
env = *environment.New(envName)
} else {
env = *ctxEnv
}
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)
}
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)
}
maps = append(maps, m)
ld.logger.Debugf("envvals_loader: loaded %s:%v", strOrMap, m)
}
}
case map[any]any, map[string]any:
maps = append(maps, strOrMap)
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 {
m, err := hclLoader.HCLRender()
if err != nil {
return nil, err
}
maps = append(maps, m)
}
result, err = mapMerge(result, maps)
if err != nil {
return nil, err
}
return result, nil
}
func mapMerge(dest map[string]any, maps []any) (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.
// See https://github.com/roboll/helmfile/issues/677
vals, err := maputil.CastKeysToStrings(m)
if err != nil {
return nil, err
}
if err := mergo.Merge(&dest, &vals, mergo.WithOverride); err != nil {
return nil, fmt.Errorf("failed to merge %v: %v", m, err)
}
}
return dest, nil
}