From d883b8dd21b209e9f60e7da2322df6ce5f3ba463 Mon Sep 17 00:00:00 2001 From: Shane Starcher Date: Wed, 13 May 2026 11:24:12 -0400 Subject: [PATCH 1/3] Prefer `helm dependency build` over `dependency up` when Chart.lock exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When chartify is engaged (e.g. helmfile invokes it for releases that use jsonPatches, strategic-merge patches, or kustomize), it copies the chart to a temp directory and runs `helm dependency up` to flatten dependencies before rendering. `dep up` re-resolves from Chart.yaml's version constraints and rewrites Chart.lock, which silently pulls newer dependency versions whenever constraints permit (e.g. `version: "*"`). This diverges from helmfile's non-chartify code path (pkg/helmexec/exec.go `BuildDeps`), which calls `helm dependency build` and honors the lock. The asymmetry means an unrelated change like adding a `jsonPatches` block to a release can flip a chart from locked-version rendering to "latest matching constraint" rendering, producing surprising image/version drift. Behavior change: - If `Chart.lock` (or legacy `requirements.lock`) exists and no adhoc chart dependencies were injected, run `helm dependency build`. This resolves dependencies from the lock and matches the rest of the helmfile pipeline. - If `dependency build` fails (typically because the lock is out of sync with Chart.yaml), fall back to `dependency up` and log the fallback. This preserves the previous escape hatch for charts whose lock has drifted. - If no lock file exists, or adhoc dependencies are present, behavior is unchanged — still `dependency up`. Adds a unit test for the lock-file detection helper. Signed-off-by: Shane Starcher --- chartify.go | 38 ++++++++++++++++++++++++++++++++++++-- util_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/chartify.go b/chartify.go index ec13432..a9c5b17 100644 --- a/chartify.go +++ b/chartify.go @@ -382,13 +382,35 @@ func (r *Runner) Chartify(release, dirOrChart string, opts ...ChartifyOption) (s "This may result in outdated chart dependencies.", release) } else { // Flatten the chart by fetching dependent chart archives and merging their K8s manifests into the temporary local chart - // So that we can uniformly patch them with JSON patch, Strategic-Merge patch, or with injectors - depArgs := []string{"dependency", "up", tempDir} + // So that we can uniformly patch them with JSON patch, Strategic-Merge patch, or with injectors. + // + // Prefer `helm dependency build` when a Chart.lock exists and no adhoc dependencies were + // injected. `build` resolves dependencies from the lock file, matching the behavior of + // helmfile's non-chartify code path (pkg/helmexec/exec.go BuildDeps) and respecting the + // reproducibility users expect when they commit a Chart.lock. `up` re-resolves against + // Chart.yaml constraints and rewrites the lock, which silently picks up newer versions + // when constraints permit (e.g. `version: "*"`). + // + // Adhoc dependencies are appended to Chart.yaml after the lock was generated, so the lock + // is by definition out of sync — `build` would fail. Fall back to `up` in that case. + useBuild := len(u.AdhocChartDependencies) == 0 && hasChartLock(tempDir) + depCmd := "up" + if useBuild { + depCmd = "build" + } + depArgs := []string{"dependency", depCmd, tempDir} // Helm 4 requires --plain-http for HTTP-only OCI registries if u.OCIPlainHTTP && r.IsHelm4() { depArgs = append(depArgs, "--plain-http") } _, err := r.run(nil, r.helmBin(), depArgs...) + if err != nil && useBuild { + // `helm dependency build` errors when Chart.lock is out of sync with Chart.yaml. + // Fall back to `up` so chartify keeps working on charts whose lock has drifted. + r.Logf("`helm dependency build` failed for release %s, falling back to `helm dependency up`: %v", release, err) + depArgs[1] = "up" + _, err = r.run(nil, r.helmBin(), depArgs...) + } if err != nil { return "", err } @@ -680,6 +702,18 @@ func (r *Runner) RewriteChartToPreventDoubleRendering(tempDir, filesDir string) return nil } +// hasChartLock reports whether a Chart.lock or requirements.lock file exists in the +// given chart directory. Helm uses Chart.lock for apiVersion v2 charts and the legacy +// requirements.lock for v1 charts; either is sufficient for `helm dependency build`. +func hasChartLock(chartDir string) bool { + for _, name := range []string{"Chart.lock", "requirements.lock"} { + if _, err := os.Stat(filepath.Join(chartDir, name)); err == nil { + return true + } + } + return false +} + func createDirForFile(f string) error { dstFileDir := filepath.Dir(f) if _, err := os.Lstat(dstFileDir); err == nil { diff --git a/util_test.go b/util_test.go index 6a944c4..9316c32 100644 --- a/util_test.go +++ b/util_test.go @@ -1,11 +1,39 @@ package chartify import ( + "os" + "path/filepath" "testing" "github.com/google/go-cmp/cmp" ) +func TestHasChartLock(t *testing.T) { + tests := []struct { + name string + lockName string // empty means no lock file is created + want bool + }{ + {name: "no lock file", lockName: "", want: false}, + {name: "Chart.lock present", lockName: "Chart.lock", want: true}, + {name: "requirements.lock present", lockName: "requirements.lock", want: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + if tt.lockName != "" { + path := filepath.Join(dir, tt.lockName) + if err := os.WriteFile(path, []byte("dependencies: []\n"), 0644); err != nil { + t.Fatalf("writing fixture: %v", err) + } + } + if got := hasChartLock(dir); got != tt.want { + t.Errorf("hasChartLock() = %v, want %v", got, tt.want) + } + }) + } +} + func TestCreateFlagChain(t *testing.T) { testcases := []struct { flag string From 929b43f14a2f3877782f9119dc6015723c2794d0 Mon Sep 17 00:00:00 2001 From: Shane Starcher Date: Sun, 17 May 2026 23:52:32 +0000 Subject: [PATCH 2/3] Address Copilot review: check deprecated deps, restrict fallback, add tests - Also check DeprecatedAdhocChartDependencies (-d flag) when deciding whether to use `helm dependency build`, preventing a guaranteed-failing build attempt followed by fallback. - Restrict the fallback from `build` to `up` to only the lock-out-of-sync error. Other failures (network, auth, missing artifacts) now surface to the caller instead of silently re-resolving dependencies. - Add TestDepCommandSelection covering all code paths: build with lock, up without lock, up with adhoc deps (both styles), fallback on sync error, no fallback on other errors, and requirements.lock support. - Add TestIsLockOutOfSyncErr for the new error classifier. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Shane Starcher --- chartify.go | 23 ++++-- chartify_test.go | 177 +++++++++++++++++++++++++++++++++++++++++++++++ util_test.go | 21 ++++++ 3 files changed, 215 insertions(+), 6 deletions(-) diff --git a/chartify.go b/chartify.go index a9c5b17..c1f2e99 100644 --- a/chartify.go +++ b/chartify.go @@ -391,9 +391,11 @@ func (r *Runner) Chartify(release, dirOrChart string, opts ...ChartifyOption) (s // Chart.yaml constraints and rewrites the lock, which silently picks up newer versions // when constraints permit (e.g. `version: "*"`). // - // Adhoc dependencies are appended to Chart.yaml after the lock was generated, so the lock - // is by definition out of sync — `build` would fail. Fall back to `up` in that case. - useBuild := len(u.AdhocChartDependencies) == 0 && hasChartLock(tempDir) + // Adhoc dependencies (both new-style and deprecated -d flag) are appended to Chart.yaml + // after the lock was generated, so the lock is by definition out of sync — `build` + // would fail. Fall back to `up` in that case. + hasAdhocDeps := len(u.AdhocChartDependencies) > 0 || len(u.DeprecatedAdhocChartDependencies) > 0 + useBuild := !hasAdhocDeps && hasChartLock(tempDir) depCmd := "up" if useBuild { depCmd = "build" @@ -404,10 +406,11 @@ func (r *Runner) Chartify(release, dirOrChart string, opts ...ChartifyOption) (s depArgs = append(depArgs, "--plain-http") } _, err := r.run(nil, r.helmBin(), depArgs...) - if err != nil && useBuild { + if err != nil && useBuild && isLockOutOfSyncErr(err) { // `helm dependency build` errors when Chart.lock is out of sync with Chart.yaml. - // Fall back to `up` so chartify keeps working on charts whose lock has drifted. - r.Logf("`helm dependency build` failed for release %s, falling back to `helm dependency up`: %v", release, err) + // Only fall back to `up` for this specific case — other errors (network, auth, + // missing artifacts) should surface to the caller rather than silently re-resolving. + r.Logf("`helm dependency build` failed for release %s (lock out of sync), falling back to `helm dependency up`: %v", release, err) depArgs[1] = "up" _, err = r.run(nil, r.helmBin(), depArgs...) } @@ -714,6 +717,14 @@ func hasChartLock(chartDir string) bool { return false } +// isLockOutOfSyncErr returns true when the error from `helm dependency build` indicates +// that the lock file is out of sync with Chart.yaml. Only this specific failure is safe +// to recover from by falling back to `helm dependency up`. +func isLockOutOfSyncErr(err error) bool { + msg := err.Error() + return strings.Contains(msg, "out of sync") || strings.Contains(msg, "lock file is out of date") +} + func createDirForFile(f string) error { dstFileDir := filepath.Dir(f) if _, err := os.Lstat(dstFileDir); err == nil { diff --git a/chartify_test.go b/chartify_test.go index b78a9d1..094dd9d 100644 --- a/chartify_test.go +++ b/chartify_test.go @@ -2,8 +2,11 @@ package chartify import ( "context" + "fmt" + "io" "os" "os/exec" + "path/filepath" "testing" "github.com/google/go-cmp/cmp" @@ -114,6 +117,180 @@ func TestReadAdhocDependencies(t *testing.T) { }) } +func TestDepCommandSelection(t *testing.T) { + newTestRunner := func(failBuild bool, failMsg string) (*Runner, *[]helmCall) { + var calls []helmCall + r := &Runner{ + HelmBinary: "helm", + isHelm3: true, + RunCommand: func(name string, args []string, dir string, stdout, stderr io.Writer, env map[string]string) error { + calls = append(calls, helmCall{name: name, args: append([]string{}, args...)}) + if failBuild && len(args) >= 2 && args[0] == "dependency" && args[1] == "build" { + if _, err := stderr.Write([]byte(failMsg)); err != nil { + return err + } + return fmt.Errorf("%s", failMsg) + } + return nil + }, + CopyFile: CopyFile, + WriteFile: os.WriteFile, + ReadFile: os.ReadFile, + ReadDir: os.ReadDir, + Walk: filepath.Walk, + Exists: exists, + Logf: func(string, ...interface{}) {}, + MakeTempDir: func(release, chart string, opts *ChartifyOpts) string { + return "" + }, + } + return r, &calls + } + + setupChart := func(t *testing.T, withLock bool, lockName string) string { + t.Helper() + dir := t.TempDir() + chartYaml := filepath.Join(dir, "Chart.yaml") + if err := os.WriteFile(chartYaml, []byte("apiVersion: v2\nname: test\nversion: 0.1.0\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(dir, "templates"), 0755); err != nil { + t.Fatal(err) + } + if withLock { + if err := os.WriteFile(filepath.Join(dir, lockName), []byte("dependencies: []\n"), 0644); err != nil { + t.Fatal(err) + } + } + return dir + } + + t.Run("uses dependency build when Chart.lock exists and no adhoc deps", func(t *testing.T) { + chartDir := setupChart(t, true, "Chart.lock") + r, calls := newTestRunner(false, "") + r.MakeTempDir = func(_, _ string, _ *ChartifyOpts) string { return chartDir } + + _, err := r.Chartify("rel", chartDir, WithChartifyOpts(&ChartifyOpts{})) + require.NoError(t, err) + + depCalls := filterDepCalls(*calls) + require.Len(t, depCalls, 1) + require.Equal(t, "build", depCalls[0].args[1]) + }) + + t.Run("uses dependency up when no lock file exists", func(t *testing.T) { + chartDir := setupChart(t, false, "") + r, calls := newTestRunner(false, "") + r.MakeTempDir = func(_, _ string, _ *ChartifyOpts) string { return chartDir } + + _, err := r.Chartify("rel", chartDir, WithChartifyOpts(&ChartifyOpts{})) + require.NoError(t, err) + + depCalls := filterDepCalls(*calls) + require.Len(t, depCalls, 1) + require.Equal(t, "up", depCalls[0].args[1]) + }) + + t.Run("uses dependency up when AdhocChartDependencies present despite lock", func(t *testing.T) { + chartDir := setupChart(t, true, "Chart.lock") + r, calls := newTestRunner(false, "") + r.MakeTempDir = func(_, _ string, _ *ChartifyOpts) string { return chartDir } + r.Exists = func(path string) (bool, error) { + if _, err := os.Stat(path); err == nil { + return true, nil + } + return false, nil + } + + _, err := r.Chartify("rel", chartDir, WithChartifyOpts(&ChartifyOpts{ + AdhocChartDependencies: []ChartDependency{{Chart: chartDir, Version: "0.1.0"}}, + })) + require.NoError(t, err) + + depCalls := filterDepCalls(*calls) + require.Len(t, depCalls, 1) + require.Equal(t, "up", depCalls[0].args[1]) + }) + + t.Run("uses dependency up when DeprecatedAdhocChartDependencies present despite lock", func(t *testing.T) { + chartDir := setupChart(t, true, "Chart.lock") + r, calls := newTestRunner(false, "") + r.MakeTempDir = func(_, _ string, _ *ChartifyOpts) string { return chartDir } + r.Exists = func(path string) (bool, error) { + if _, err := os.Stat(path); err == nil { + return true, nil + } + return false, nil + } + + _, err := r.Chartify("rel", chartDir, WithChartifyOpts(&ChartifyOpts{ + DeprecatedAdhocChartDependencies: []string{chartDir + ":0.1.0"}, + })) + require.NoError(t, err) + + depCalls := filterDepCalls(*calls) + require.Len(t, depCalls, 1) + require.Equal(t, "up", depCalls[0].args[1]) + }) + + t.Run("falls back to up when build fails with lock out of sync", func(t *testing.T) { + chartDir := setupChart(t, true, "Chart.lock") + r, calls := newTestRunner(true, "the lock file (Chart.lock) is out of sync with the dependencies listed in Chart.yaml") + r.MakeTempDir = func(_, _ string, _ *ChartifyOpts) string { return chartDir } + + _, err := r.Chartify("rel", chartDir, WithChartifyOpts(&ChartifyOpts{})) + require.NoError(t, err) + + depCalls := filterDepCalls(*calls) + require.Len(t, depCalls, 2) + require.Equal(t, "build", depCalls[0].args[1]) + require.Equal(t, "up", depCalls[1].args[1]) + }) + + t.Run("does not fall back to up when build fails with non-sync error", func(t *testing.T) { + chartDir := setupChart(t, true, "Chart.lock") + r, calls := newTestRunner(true, "network timeout fetching dependency") + r.MakeTempDir = func(_, _ string, _ *ChartifyOpts) string { return chartDir } + + _, err := r.Chartify("rel", chartDir, WithChartifyOpts(&ChartifyOpts{})) + require.Error(t, err) + require.Contains(t, err.Error(), "network timeout") + + depCalls := filterDepCalls(*calls) + require.Len(t, depCalls, 1) + require.Equal(t, "build", depCalls[0].args[1]) + _ = calls + }) + + t.Run("uses requirements.lock for legacy charts", func(t *testing.T) { + chartDir := setupChart(t, true, "requirements.lock") + r, calls := newTestRunner(false, "") + r.MakeTempDir = func(_, _ string, _ *ChartifyOpts) string { return chartDir } + + _, err := r.Chartify("rel", chartDir, WithChartifyOpts(&ChartifyOpts{})) + require.NoError(t, err) + + depCalls := filterDepCalls(*calls) + require.Len(t, depCalls, 1) + require.Equal(t, "build", depCalls[0].args[1]) + }) +} + +type helmCall struct { + name string + args []string +} + +func filterDepCalls(calls []helmCall) []helmCall { + var result []helmCall + for _, c := range calls { + if len(c.args) >= 2 && c.args[0] == "dependency" { + result = append(result, c) + } + } + return result +} + func TestUseHelmChartsInKustomize(t *testing.T) { repo := "myrepo" startServer(t, repo) diff --git a/util_test.go b/util_test.go index 9316c32..8f171e8 100644 --- a/util_test.go +++ b/util_test.go @@ -1,6 +1,7 @@ package chartify import ( + "fmt" "os" "path/filepath" "testing" @@ -34,6 +35,26 @@ func TestHasChartLock(t *testing.T) { } } +func TestIsLockOutOfSyncErr(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {name: "out of sync message", err: fmt.Errorf("the lock file (Chart.lock) is out of sync with the dependencies listed in Chart.yaml"), want: true}, + {name: "lock file is out of date", err: fmt.Errorf("lock file is out of date"), want: true}, + {name: "network error", err: fmt.Errorf("network timeout fetching dependency"), want: false}, + {name: "auth error", err: fmt.Errorf("401 unauthorized"), want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isLockOutOfSyncErr(tt.err); got != tt.want { + t.Errorf("isLockOutOfSyncErr() = %v, want %v", got, tt.want) + } + }) + } +} + func TestCreateFlagChain(t *testing.T) { testcases := []struct { flag string From 538def80012767faff56ad99d47b3ca2e3e71d79 Mon Sep 17 00:00:00 2001 From: Shane Starcher Date: Mon, 18 May 2026 00:01:38 +0000 Subject: [PATCH 3/3] Trim verbose comment block, remove dead code, document Helm version strings - Condense the 12-line inline rationale to 3 lines (PR description has the full context) - Remove leftover `_ = calls` in TestDepCommandSelection - Add version provenance comment to isLockOutOfSyncErr Co-Authored-By: Claude Opus 4.6 Signed-off-by: Shane Starcher --- chartify.go | 15 ++++----------- chartify_test.go | 1 - 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/chartify.go b/chartify.go index c1f2e99..b174408 100644 --- a/chartify.go +++ b/chartify.go @@ -382,18 +382,10 @@ func (r *Runner) Chartify(release, dirOrChart string, opts ...ChartifyOption) (s "This may result in outdated chart dependencies.", release) } else { // Flatten the chart by fetching dependent chart archives and merging their K8s manifests into the temporary local chart - // So that we can uniformly patch them with JSON patch, Strategic-Merge patch, or with injectors. + // so that we can uniformly patch them with JSON patch, Strategic-Merge patch, or with injectors. // - // Prefer `helm dependency build` when a Chart.lock exists and no adhoc dependencies were - // injected. `build` resolves dependencies from the lock file, matching the behavior of - // helmfile's non-chartify code path (pkg/helmexec/exec.go BuildDeps) and respecting the - // reproducibility users expect when they commit a Chart.lock. `up` re-resolves against - // Chart.yaml constraints and rewrites the lock, which silently picks up newer versions - // when constraints permit (e.g. `version: "*"`). - // - // Adhoc dependencies (both new-style and deprecated -d flag) are appended to Chart.yaml - // after the lock was generated, so the lock is by definition out of sync — `build` - // would fail. Fall back to `up` in that case. + // Use `helm dependency build` (honors Chart.lock) when a lock exists and no adhoc deps + // were injected; otherwise fall back to `helm dependency up` (re-resolves from Chart.yaml). hasAdhocDeps := len(u.AdhocChartDependencies) > 0 || len(u.DeprecatedAdhocChartDependencies) > 0 useBuild := !hasAdhocDeps && hasChartLock(tempDir) depCmd := "up" @@ -720,6 +712,7 @@ func hasChartLock(chartDir string) bool { // isLockOutOfSyncErr returns true when the error from `helm dependency build` indicates // that the lock file is out of sync with Chart.yaml. Only this specific failure is safe // to recover from by falling back to `helm dependency up`. +// Matches messages from Helm 3.x ("out of sync") and Helm 2.x ("lock file is out of date"). func isLockOutOfSyncErr(err error) bool { msg := err.Error() return strings.Contains(msg, "out of sync") || strings.Contains(msg, "lock file is out of date") diff --git a/chartify_test.go b/chartify_test.go index 094dd9d..1a559f2 100644 --- a/chartify_test.go +++ b/chartify_test.go @@ -259,7 +259,6 @@ func TestDepCommandSelection(t *testing.T) { depCalls := filterDepCalls(*calls) require.Len(t, depCalls, 1) require.Equal(t, "build", depCalls[0].args[1]) - _ = calls }) t.Run("uses requirements.lock for legacy charts", func(t *testing.T) {