From d3908e6a3c179a38ad8f416a1ff715431bb93543 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sun, 26 Oct 2025 19:41:39 -0400 Subject: [PATCH] Fix helmBinary and kustomizeBinary being ignored when using bases (#2228) * Fix helmBinary and kustomizeBinary being ignored when using bases - Add mergo.WithOverride to merge operations for proper precedence - Move default binary setting after base loading - Add comprehensive tests for various base scenarios Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> * Fix code formatting in create_test.go Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> * Remove duplicate comment block in create_test.go Removed duplicate comment lines (530-532) as identified by code review. Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> --- pkg/state/create.go | 22 +++-- pkg/state/create_test.go | 203 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+), 7 deletions(-) diff --git a/pkg/state/create.go b/pkg/state/create.go index 97e91823..606134fd 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -116,11 +116,19 @@ func (c *StateCreator) Parse(content []byte, baseDir, file string) (*HelmState, return nil, &StateLoadError{fmt.Sprintf("failed to read %s: reading document at index %d", file, i), err} } - if err := mergo.Merge(&state, &intermediate, mergo.WithAppendSlice); err != nil { + if err := mergo.Merge(&state, &intermediate, mergo.WithAppendSlice, mergo.WithOverride); err != nil { return nil, &StateLoadError{fmt.Sprintf("failed to read %s: merging document at index %d", file, i), err} } } + state.logger = c.logger + state.valsRuntime = c.valsRuntime + + return &state, nil +} + +// applyDefaultsAndOverrides applies default binary paths and command-line overrides +func (c *StateCreator) applyDefaultsAndOverrides(state *HelmState) { if c.overrideHelmBinary != "" && c.overrideHelmBinary != DefaultHelmBinary { state.DefaultHelmBinary = c.overrideHelmBinary } else if state.DefaultHelmBinary == "" { @@ -134,11 +142,6 @@ func (c *StateCreator) Parse(content []byte, baseDir, file string) (*HelmState, // Let `helmfile --kustomize-binary ""` not break this helmfile run state.DefaultKustomizeBinary = DefaultKustomizeBinary } - - state.logger = c.logger - state.valsRuntime = c.valsRuntime - - return &state, nil } // LoadEnvValues loads environment values files relative to the `baseDir` @@ -181,6 +184,11 @@ func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envNam if err != nil { return nil, err } + + // Apply default binaries and command-line overrides only for the main helmfile + // after loading and merging all bases. This ensures that values from bases are + // properly respected and that later bases/documents can override earlier ones. + c.applyDefaultsAndOverrides(state) } state, err = c.LoadEnvValues(state, envName, failOnMissingEnv, envValues, overrode) @@ -216,7 +224,7 @@ func (c *StateCreator) loadBases(envValues, overrodeEnv *environment.Environment layers = append(layers, st) for i := 1; i < len(layers); i++ { - if err := mergo.Merge(layers[0], layers[i], mergo.WithAppendSlice); err != nil { + if err := mergo.Merge(layers[0], layers[i], mergo.WithAppendSlice, mergo.WithOverride); err != nil { return nil, err } } diff --git a/pkg/state/create_test.go b/pkg/state/create_test.go index 29ac5ca8..e8394458 100644 --- a/pkg/state/create_test.go +++ b/pkg/state/create_test.go @@ -1,6 +1,7 @@ package state import ( + "fmt" "path/filepath" "reflect" "testing" @@ -525,3 +526,205 @@ releaseContext: t.Errorf("unexpected values: expected=%v, actual=%v", expectedValues, actualValues) } } + +// TestHelmBinaryInBases tests that helmBinary and kustomizeBinary settings +// from bases are properly merged with later values overriding earlier ones +func TestHelmBinaryInBases(t *testing.T) { + tests := []struct { + name string + files map[string]string + mainFile string + expectedHelmBinary string + expectedKustomizeBinary string + }{ + { + name: "helmBinary in second base should be used", + files: map[string]string{ + "/path/to/helmfile.yaml": `bases: + - ./bases/env.yaml +--- +bases: + - ./bases/repos.yaml +--- +bases: + - ./bases/releases.yaml +`, + "/path/to/bases/env.yaml": `environments: + default: + values: + - key: value1 +`, + "/path/to/bases/repos.yaml": `repositories: + - name: stable + url: https://charts.helm.sh/stable +helmBinary: /path/to/custom/helm +`, + "/path/to/bases/releases.yaml": `releases: + - name: myapp + chart: stable/nginx +`, + }, + mainFile: "/path/to/helmfile.yaml", + expectedHelmBinary: "/path/to/custom/helm", + expectedKustomizeBinary: DefaultKustomizeBinary, + }, + { + name: "helmBinary in main file after bases should override", + files: map[string]string{ + "/path/to/helmfile.yaml": `bases: + - ./bases/env.yaml +--- +bases: + - ./bases/repos.yaml +--- +bases: + - ./bases/releases.yaml +helmBinary: /path/to/main/helm +`, + "/path/to/bases/env.yaml": `environments: + default: + values: + - key: value1 +`, + "/path/to/bases/repos.yaml": `repositories: + - name: stable + url: https://charts.helm.sh/stable +helmBinary: /path/to/base/helm +`, + "/path/to/bases/releases.yaml": `releases: + - name: myapp + chart: stable/nginx +`, + }, + mainFile: "/path/to/helmfile.yaml", + expectedHelmBinary: "/path/to/main/helm", + expectedKustomizeBinary: DefaultKustomizeBinary, + }, + { + name: "helmBinary in main file between bases should override earlier bases", + files: map[string]string{ + "/path/to/helmfile.yaml": `bases: + - ./bases/env.yaml +--- +bases: + - ./bases/repos.yaml +helmBinary: /path/to/middle/helm +--- +bases: + - ./bases/releases.yaml +`, + "/path/to/bases/env.yaml": `environments: + default: + values: + - key: value1 +`, + "/path/to/bases/repos.yaml": `repositories: + - name: stable + url: https://charts.helm.sh/stable +helmBinary: /path/to/base/helm +`, + "/path/to/bases/releases.yaml": `releases: + - name: myapp + chart: stable/nginx +`, + }, + mainFile: "/path/to/helmfile.yaml", + expectedHelmBinary: "/path/to/middle/helm", + expectedKustomizeBinary: DefaultKustomizeBinary, + }, + { + name: "kustomizeBinary in base should be used", + files: map[string]string{ + "/path/to/helmfile.yaml": `bases: + - ./bases/base.yaml +`, + "/path/to/bases/base.yaml": `kustomizeBinary: /path/to/custom/kustomize +releases: + - name: myapp + chart: mychart +`, + }, + mainFile: "/path/to/helmfile.yaml", + expectedHelmBinary: DefaultHelmBinary, + expectedKustomizeBinary: "/path/to/custom/kustomize", + }, + { + name: "both helmBinary and kustomizeBinary in different bases", + files: map[string]string{ + "/path/to/helmfile.yaml": `bases: + - ./bases/helm.yaml +--- +bases: + - ./bases/kustomize.yaml +`, + "/path/to/bases/helm.yaml": `helmBinary: /path/to/custom/helm +`, + "/path/to/bases/kustomize.yaml": `kustomizeBinary: /path/to/custom/kustomize +`, + }, + mainFile: "/path/to/helmfile.yaml", + expectedHelmBinary: "/path/to/custom/helm", + expectedKustomizeBinary: "/path/to/custom/kustomize", + }, + { + name: "later base overrides earlier base for helmBinary", + files: map[string]string{ + "/path/to/helmfile.yaml": `bases: + - ./bases/first.yaml +--- +bases: + - ./bases/second.yaml +`, + "/path/to/bases/first.yaml": `helmBinary: /path/to/first/helm +`, + "/path/to/bases/second.yaml": `helmBinary: /path/to/second/helm +`, + }, + mainFile: "/path/to/helmfile.yaml", + expectedHelmBinary: "/path/to/second/helm", + expectedKustomizeBinary: DefaultKustomizeBinary, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testFs := testhelper.NewTestFs(tt.files) + if testFs.Cwd == "" { + testFs.Cwd = "/" + } + + r := remote.NewRemote(logger, testFs.Cwd, testFs.ToFileSystem()) + creator := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", "", r, false, "") + + // Set up LoadFile for recursive base loading + creator.LoadFile = func(inheritedEnv, overrodeEnv *environment.Environment, baseDir, file string, evaluateBases bool) (*HelmState, error) { + path := filepath.Join(baseDir, file) + content, ok := tt.files[path] + if !ok { + return nil, fmt.Errorf("file not found: %s", path) + } + return creator.ParseAndLoad([]byte(content), filepath.Dir(path), path, DefaultEnv, true, evaluateBases, inheritedEnv, overrodeEnv) + } + + yamlContent, ok := tt.files[tt.mainFile] + if !ok { + t.Fatalf("no file named %q registered", tt.mainFile) + } + + state, err := creator.ParseAndLoad([]byte(yamlContent), filepath.Dir(tt.mainFile), tt.mainFile, DefaultEnv, true, true, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if state.DefaultHelmBinary != tt.expectedHelmBinary { + t.Errorf("helmBinary mismatch: expected=%s, actual=%s", + tt.expectedHelmBinary, state.DefaultHelmBinary) + } + + if state.DefaultKustomizeBinary != tt.expectedKustomizeBinary { + t.Errorf("kustomizeBinary mismatch: expected=%s, actual=%s", + tt.expectedKustomizeBinary, state.DefaultKustomizeBinary) + } + }) + } +}