mirror of
https://github.com/helmfile/helmfile.git
synced 2026-09-30 12:34:23 +02:00
* 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>
288 lines
7.8 KiB
Go
288 lines
7.8 KiB
Go
package state
|
|
|
|
import (
|
|
"fmt"
|
|
"io"
|
|
"os"
|
|
"path/filepath"
|
|
"reflect"
|
|
"testing"
|
|
|
|
"github.com/helmfile/helmfile/pkg/filesystem"
|
|
"github.com/helmfile/helmfile/pkg/helmexec"
|
|
"github.com/helmfile/helmfile/pkg/remote"
|
|
)
|
|
|
|
func TestStorage_resolveFile(t *testing.T) {
|
|
type args struct {
|
|
missingFileHandler *string
|
|
title string
|
|
path string
|
|
opts []resolveFileOption
|
|
}
|
|
|
|
cacheDir := remote.CacheDir()
|
|
infoHandler := MissingFileHandlerInfo
|
|
warnHandler := MissingFileHandlerWarn
|
|
errorHandler := MissingFileHandlerError
|
|
|
|
tests := []struct {
|
|
name string
|
|
args args
|
|
wantFiles []string
|
|
wantSkipped bool
|
|
wantErr bool
|
|
skipNonCI bool
|
|
}{
|
|
{
|
|
name: "non existing file in repo produce skip",
|
|
args: args{
|
|
path: "git::https://github.com/helmfile/helmfile.git@examples/values/non-existing-file.yaml?ref=v0.145.2",
|
|
title: "values",
|
|
missingFileHandler: &infoHandler,
|
|
},
|
|
wantSkipped: true,
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "non existing file in repo produce skip",
|
|
args: args{
|
|
path: "git::https://github.com/helmfile/helmfile.git@examples/values/non-existing-file.yaml?ref=v0.145.2",
|
|
title: "values",
|
|
missingFileHandler: &errorHandler,
|
|
},
|
|
wantSkipped: false,
|
|
wantErr: true,
|
|
},
|
|
{
|
|
name: "non existing branch in repo produce error",
|
|
args: args{
|
|
path: "git::https://github.com/helmfile/helmfile.git@examples/values/non-existing-file.yaml?ref=inexistent-branch-for-test",
|
|
title: "values",
|
|
missingFileHandler: &infoHandler,
|
|
},
|
|
wantSkipped: false,
|
|
wantErr: true,
|
|
skipNonCI: true,
|
|
},
|
|
{
|
|
name: "non existing branch in repo produce info when ignoreMissingGitBranch=true",
|
|
args: args{
|
|
path: "git::https://github.com/helmfile/helmfile.git@examples/values/non-existing-file.yaml?ref=inexistent-branch-for-test",
|
|
title: "values",
|
|
missingFileHandler: &infoHandler,
|
|
opts: []resolveFileOption{
|
|
ignoreMissingGitBranch(true),
|
|
},
|
|
},
|
|
wantSkipped: true,
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "non existing branch in repo produce warn when ignoreMissingGitBranch=true",
|
|
args: args{
|
|
path: "git::https://github.com/helmfile/helmfile.git@examples/values/non-existing-file.yaml?ref=inexistent-branch-for-test",
|
|
title: "values",
|
|
missingFileHandler: &warnHandler,
|
|
opts: []resolveFileOption{
|
|
ignoreMissingGitBranch(true),
|
|
},
|
|
},
|
|
wantSkipped: true,
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "non existing branch in repo produce error with error handler even if ignoreMissingGitBranch=true",
|
|
args: args{
|
|
path: "git::https://github.com/helmfile/helmfile.git@examples/values/non-existing-file.yaml?ref=inexistent-branch-for-test",
|
|
title: "values",
|
|
missingFileHandler: &errorHandler,
|
|
opts: []resolveFileOption{
|
|
ignoreMissingGitBranch(true),
|
|
},
|
|
},
|
|
wantSkipped: false,
|
|
wantErr: true,
|
|
},
|
|
{
|
|
name: "existing remote value fetched",
|
|
args: args{
|
|
path: "git::https://github.com/helmfile/helmfile.git@examples/values/replica-values.yaml?ref=v0.145.2",
|
|
title: "values",
|
|
missingFileHandler: &infoHandler,
|
|
},
|
|
wantFiles: []string{fmt.Sprintf("%s/%s", cacheDir, "values/https_github_com_helmfile_helmfile_git.ref=v0.145.2/examples/values/replica-values.yaml")},
|
|
wantSkipped: false,
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "non existing remote repo produce an error",
|
|
args: args{
|
|
path: "https://github.com/helmfile/helmfiles.git@examples/values/replica-values.yaml?ref=v0.145.2",
|
|
title: "values",
|
|
missingFileHandler: &infoHandler,
|
|
},
|
|
wantSkipped: false,
|
|
wantErr: true,
|
|
},
|
|
}
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
if tt.skipNonCI && os.Getenv("CI") == "" {
|
|
// CI uses HTTPS git remotes while local dev often has SSH configured via
|
|
// git config url."ssh://".insteadOf. This causes different behavior for
|
|
// non-existent branch scenarios.
|
|
t.Skip("skipping test that requires CI environment (git SSH/HTTPS differences)")
|
|
}
|
|
|
|
st := NewStorage(cacheDir, helmexec.NewLogger(io.Discard, "debug"), filesystem.DefaultFileSystem())
|
|
|
|
files, skipped, err := st.resolveFile(tt.args.missingFileHandler, tt.args.title, tt.args.path, tt.args.opts...)
|
|
if (err != nil) != tt.wantErr {
|
|
t.Errorf("resolveFile() error = %v, wantErr %v", err, tt.wantErr)
|
|
return
|
|
}
|
|
if !reflect.DeepEqual(files, tt.wantFiles) {
|
|
t.Errorf("resolveFile() files = %v, want %v", files, tt.wantFiles)
|
|
}
|
|
if skipped != tt.wantSkipped {
|
|
t.Errorf("resolveFile() skipped = %v, want %v", skipped, tt.wantSkipped)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestNormalizePath(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
base string
|
|
path string
|
|
want string
|
|
}{
|
|
{
|
|
name: "unix path relative path",
|
|
base: "/root",
|
|
path: "local/timespan-application.yml",
|
|
want: "/local/timespan-application.yml",
|
|
},
|
|
{
|
|
name: "unix path absolute path",
|
|
base: "/data",
|
|
path: "/root/data/timespan-application.yml",
|
|
want: "/root/data/timespan-application.yml",
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
storageIns := NewStorage(tt.base, helmexec.NewLogger(io.Discard, "debug"), filesystem.DefaultFileSystem())
|
|
if got := storageIns.normalizePath(tt.path); got != tt.want {
|
|
t.Errorf("normalizePath() = %v, want %v", got, tt.want)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestJoinBase(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
base string
|
|
path string
|
|
want string
|
|
}{
|
|
{
|
|
name: "joinBase with non-root base",
|
|
base: "/root",
|
|
path: "local/timespan-application.yml",
|
|
want: "/local/timespan-application.yml",
|
|
},
|
|
{
|
|
name: "joinBase with root path",
|
|
base: "/",
|
|
path: "data/timespan-application.yml",
|
|
want: "/data/timespan-application.yml",
|
|
},
|
|
{
|
|
name: "windows joinBase",
|
|
base: "",
|
|
path: "data\\timespan-application.yml",
|
|
want: "data\\timespan-application.yml",
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
storageIns := NewStorage(tt.base, helmexec.NewLogger(io.Discard, "debug"), filesystem.DefaultFileSystem())
|
|
if got := storageIns.JoinBase(tt.path); got != tt.want {
|
|
t.Errorf("JoinBase() = %v, want %v", got, tt.want)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestNormalizeSetFilePath(t *testing.T) {
|
|
st := &Storage{
|
|
basePath: "/base/path",
|
|
}
|
|
|
|
tests := []struct {
|
|
name string
|
|
path string
|
|
expected string
|
|
osGOOS string
|
|
}{
|
|
{
|
|
name: "Unix path on Unix",
|
|
path: "relative/path",
|
|
expected: "/base/path/relative/path",
|
|
osGOOS: "linux",
|
|
},
|
|
{
|
|
name: "Windows path on Windows",
|
|
path: "relative\\path",
|
|
expected: "/base/path/relative\\\\path",
|
|
osGOOS: "windows",
|
|
},
|
|
{
|
|
name: "Unix path on Windows",
|
|
path: "relative/path",
|
|
expected: "/base/path/relative/path",
|
|
osGOOS: "windows",
|
|
},
|
|
{
|
|
name: "Absolute path on Unix",
|
|
path: "/absolute/path",
|
|
expected: "/absolute/path",
|
|
osGOOS: "linux",
|
|
},
|
|
{
|
|
name: "Absolute path on Windows",
|
|
path: "C:\\absolute\\path",
|
|
expected: "C:\\\\absolute\\\\path",
|
|
osGOOS: "windows",
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
result := st.normalizeSetFilePath(tt.path, tt.osGOOS)
|
|
if tt.osGOOS == "windows" {
|
|
if result != tt.expected {
|
|
t.Errorf("normalizeSetFilePath() = %v, want %v", result, tt.expected)
|
|
}
|
|
} else {
|
|
expectedPath := filepath.Join(st.basePath, tt.path)
|
|
if !filepath.IsAbs(tt.path) {
|
|
if result != expectedPath {
|
|
t.Errorf("normalizeSetFilePath() = %v, want %v", result, expectedPath)
|
|
}
|
|
} else {
|
|
if result != tt.path {
|
|
t.Errorf("normalizeSetFilePath() = %v, want %v", result, tt.path)
|
|
}
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|