fix: route prebuild dep builds through the helmexec runner

Address Copilot review comments on preBuildTransitiveSubchartDeps: - execHelmDep shelled out via os/exec, bypassing Helmfile's standard command wiring. Thread the helmexec.Interface (already available in prepareChartForRelease) through processChartification into the prebuild helpers and dispatch via helm.BuildDeps/helm.UpdateDeps. Drop the os/exec call and the DefaultHelmBinary re-resolution. - isLockOutOfSync now inspects the helmexec ExitError message (which embeds the combined output) instead of raw []byte output. - DoesNotRecurseIntoRemoteDeps used a real helm binary, risking network I/O to https://charts.example.com. Switch it to a stub helm whose BuildDeps returns an error so it fails fast and deterministically, and assert it visits only the chart dir (no recursion). Signed-off-by: yxxhero <aiopsclub@163.com>
2026-10-04 23:21:35 +02:00 · 2026-07-02 06:37:50 +08:00
parent bfcabf3ead
commit 6ccfe559b1
2 changed files with 92 additions and 49 deletions
@@ -1,6 +1,8 @@
 package state

 import (
+	"context"
+	"errors"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -11,11 +13,12 @@ import (
 	"go.uber.org/zap"

 	"github.com/helmfile/helmfile/pkg/filesystem"
+	"github.com/helmfile/helmfile/pkg/helmexec"
 )

 // skipIfNoHelm skips the calling test when the helm binary is not available on
-// PATH. Tests that exercise runHelmDepBuild need helm installed to produce
-// meaningful results.
+// PATH. Tests that exercise runHelmDepBuild through a real helmexec need helm
+// installed to produce meaningful results.
 func skipIfNoHelm(t *testing.T) {
 	t.Helper()
 	if _, err := exec.LookPath("helm"); err != nil {
@@ -23,6 +26,42 @@ func skipIfNoHelm(t *testing.T) {
 	}
 }

+// newRealHelmExec builds a helmexec.Interface backed by the real helm binary on
+// PATH, for tests that assert actual `helm dependency build` side effects.
+func newRealHelmExec(t *testing.T) helmexec.Interface {
+	t.Helper()
+	logger := zap.NewNop().Sugar()
+	runner := &helmexec.ShellRunner{Ctx: context.Background(), Logger: logger}
+	helm, err := helmexec.New("helm", helmexec.HelmExecOptions{}, logger, "", "", runner)
+	if err != nil {
+		t.Fatalf("helmexec.New: %v", err)
+	}
+	return helm
+}
+
+// stubHelm is a minimal helmexec.Interface for prebuild tests that should NOT
+// invoke a real helm binary (avoids network I/O and the helm dependency). It
+// records BuildDeps/UpdateDeps calls so tests can assert which charts were
+// visited. Only BuildDeps/UpdateDeps are exercised by the prebuild path, so the
+// embedded nil Interface satisfies the rest of the contract without being
+// dereferenced.
+type stubHelm struct {
+	helmexec.Interface
+	buildErr error
+	builds   []string
+	updates  []string
+}
+
+func (s *stubHelm) BuildDeps(name, chart string, flags ...string) error {
+	s.builds = append(s.builds, chart)
+	return s.buildErr
+}
+
+func (s *stubHelm) UpdateDeps(chart string) error {
+	s.updates = append(s.updates, chart)
+	return s.buildErr
+}
+
 // writeChart creates a chart directory with Chart.yaml and the given templates.
 // deps maps dependency name → repository URL (skipped if empty).
 func writeChart(t *testing.T, dir, name string, deps map[string]string, templates map[string]string) {
@@ -97,7 +136,7 @@ func TestPreBuildTransitiveSubchartDeps_VisitsAllTransitiveDeps(t *testing.T) {
 		},
 	}

-	st.preBuildTransitiveSubchartDeps(filepath.Join(rootDir, "envelope"))
+	st.preBuildTransitiveSubchartDeps(filepath.Join(rootDir, "envelope"), newRealHelmExec(t))

 	// The critical assertion: sub2/charts/ should now contain nested (as
 	// .tgz or unpacked). Without the fix, helm dep build on the parent chart
@@ -138,7 +177,8 @@ func hasNestedTgzPrefix(name string) bool {
 }

 // TestPreBuildTransitiveSubchartDeps_HandlesMissingChartYaml verifies the
-// function is a no-op (no panic) when the chart dir has no Chart.yaml.
+// function is a no-op (no panic) when the chart dir has no Chart.yaml. A stub
+// helm is used since the missing Chart.yaml short-circuits before any helm call.
 func TestPreBuildTransitiveSubchartDeps_HandlesMissingChartYaml(t *testing.T) {
 	rootDir, err := os.MkdirTemp("", "helmfile-prebuild-noyaml-")
 	if err != nil {
@@ -155,7 +195,7 @@ func TestPreBuildTransitiveSubchartDeps_HandlesMissingChartYaml(t *testing.T) {
 	}

 	// Should not panic.
-	st.preBuildTransitiveSubchartDeps(rootDir)
+	st.preBuildTransitiveSubchartDeps(rootDir, &stubHelm{})
 }

 // TestPreBuildTransitiveSubchartDeps_HandlesCircularDeps verifies the function
@@ -190,7 +230,7 @@ func TestPreBuildTransitiveSubchartDeps_HandlesCircularDeps(t *testing.T) {
 	done := make(chan struct{})
 	go func() {
 		defer close(done)
-		st.preBuildTransitiveSubchartDeps(aDir)
+		st.preBuildTransitiveSubchartDeps(aDir, newRealHelmExec(t))
 	}()
 	select {
 	case <-done:
@@ -202,8 +242,12 @@ func TestPreBuildTransitiveSubchartDeps_HandlesCircularDeps(t *testing.T) {

 // TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps verifies the
 // function does not recurse into dependencies declared with non-file://
-// repositories (https://, oci://). It still runs helm dep build on the chart
-// dir itself, but the failure to resolve the remote is logged and swallowed.
+// repositories (https://, oci://). It still runs helm dep build on the chart dir
+// itself, but does not follow the remote dependency.
+//
+// A stub helm whose BuildDeps returns an error is used so the test fails fast
+// and deterministically without performing any network I/O (a real helm would
+// try to fetch https://charts.example.com).
 func TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps(t *testing.T) {
 	rootDir, err := os.MkdirTemp("", "helmfile-prebuild-remote-")
 	if err != nil {
@@ -212,9 +256,9 @@ func TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps(t *testing.
 	defer os.RemoveAll(rootDir)

 	// Chart with only a remote dep. preBuildTransitiveSubchartDeps should not
-	// recurse into anything (no file:// deps to follow) and should not panic
-	// when helm dep build can't resolve the remote dep in the test environment.
-	writeChart(t, filepath.Join(rootDir, "chart"), "chart",
+	// recurse into anything (no file:// deps to follow).
+	chartDir := filepath.Join(rootDir, "chart")
+	writeChart(t, chartDir, "chart",
 		map[string]string{"remote": "https://charts.example.com"},
 		nil,
 	)
@@ -227,6 +271,20 @@ func TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps(t *testing.
 		},
 	}

-	// Should not panic; helm dep build failure is logged and swallowed.
-	st.preBuildTransitiveSubchartDeps(filepath.Join(rootDir, "chart"))
+	helm := &stubHelm{buildErr: errors.New("stub: dependency build unavailable")}
+	st.preBuildTransitiveSubchartDeps(chartDir, helm)
+
+	// BuildDeps must be called exactly once, on the chart dir itself — proving
+	// the function did not recurse into the remote dep.
+	if len(helm.builds) != 1 {
+		t.Fatalf("expected exactly one BuildDeps call (chart dir, no recursion into remote), got %v", helm.builds)
+	}
+	if got, want := filepath.Base(helm.builds[0]), "chart"; got != want {
+		t.Fatalf("BuildDeps called on %q, want chart dir %q", helm.builds[0], want)
+	}
+	// The stub error is not a lock-out-of-sync signal, so there must be no
+	// fallback to UpdateDeps.
+	if len(helm.updates) != 0 {
+		t.Fatalf("expected no UpdateDeps fallback, got %v", helm.updates)
+	}
 }
@@ -12,7 +12,6 @@ import (
 	"io"
 	"net/url"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -1846,13 +1845,13 @@ func (st *HelmState) rewriteChartDependencies(chartPath string) (string, func(),
 //
 // Best-effort: errors are logged, not returned, so a single broken subchart
 // doesn't abort the entire release.
-func (st *HelmState) preBuildTransitiveSubchartDeps(chartPath string) {
-	st.prebuildChartDeps(chartPath, make(map[string]bool))
+func (st *HelmState) preBuildTransitiveSubchartDeps(chartPath string, helm helmexec.Interface) {
+	st.prebuildChartDeps(chartPath, make(map[string]bool), helm)
 }

 // prebuildChartDeps is the recursive worker. It visits each file:// dependency
 // depth-first, then runs helm dependency build on chartDir itself.
-func (st *HelmState) prebuildChartDeps(chartDir string, visited map[string]bool) {
+func (st *HelmState) prebuildChartDeps(chartDir string, visited map[string]bool, helm helmexec.Interface) {
 	absChartDir, err := filepath.Abs(chartDir)
 	if err != nil || visited[absChartDir] {
 		return
@@ -1864,9 +1863,9 @@ func (st *HelmState) prebuildChartDeps(chartDir string, visited map[string]bool)
 		return
 	}
 	for _, depDir := range deps {
-		st.prebuildChartDeps(depDir, visited)
+		st.prebuildChartDeps(depDir, visited, helm)
 	}
-	st.runHelmDepBuild(absChartDir)
+	st.runHelmDepBuild(absChartDir, helm)
 }

 // parseLocalFileDeps reads chartDir/Chart.yaml and returns the absolute paths
@@ -1920,48 +1919,34 @@ func (st *HelmState) resolveFileDep(repo, parentDir string) (string, bool) {

 // runHelmDepBuild runs `helm dependency build` on chartDir with a per-dir lock
 // to prevent concurrent releases from corrupting charts/*.tgz (same pattern as
-// forcedDownloadChart for #768). Falls back to `helm dependency update` when
-// Chart.lock is missing or stale.
+// forcedDownloadChart for #768). It dispatches through the shared helmexec
+// runner so the command inherits Helmfile's standard wiring (binary resolution,
+// logging, flag handling, context cancellation) instead of shelling out
+// directly. Falls back to `helm dependency update` when Chart.lock is missing or
+// stale.
 //
 // --skip-refresh avoids a slow `helm repo update` per subchart; file:// deps
 // (the envelope-chart use case) don't need the repo cache. For mixed file:// +
 // https:// deps, chartify's own helm dep build (without --skip-refresh) runs
 // afterwards as a safety net.
-func (st *HelmState) runHelmDepBuild(chartDir string) {
+func (st *HelmState) runHelmDepBuild(chartDir string, helm helmexec.Interface) {
 	mu := st.getNamedRWMutex("prebuild-deps:" + chartDir)
 	mu.Lock()
 	defer mu.Unlock()

-	helmBin := st.DefaultHelmBinary
-	if helmBin == "" {
-		helmBin = "helm"
-	}
-
 	// Try `dependency build` first (honors Chart.lock). Fall back to `update`
-	// (re-resolves from Chart.yaml) when the lock is missing or stale.
-	output, err := st.execHelmDep(helmBin, "build", chartDir)
-	if err != nil && isLockOutOfSync(output) {
+	// (re-resolves from Chart.yaml) when the lock is missing or stale. BuildDeps
+	// returns a helmexec.ExitError whose message embeds the combined output, so
+	// isLockOutOfSync can inspect err.Error() without a separate os/exec call.
+	if err := helm.BuildDeps(filepath.Base(chartDir), chartDir, "--skip-refresh"); err != nil && isLockOutOfSync(err.Error()) {
 		st.logger.Debugf("runHelmDepBuild: falling back to `dependency update` for %s", chartDir)
-		_, _ = st.execHelmDep(helmBin, "update", chartDir)
+		_ = helm.UpdateDeps(chartDir)
 	}
 }

-// execHelmDep executes `helm dependency <subcmd> <chartDir> --skip-refresh`,
-// logs the result, and returns the combined output and error.
-func (st *HelmState) execHelmDep(helmBin, subcmd, chartDir string) ([]byte, error) {
-	output, err := exec.Command(helmBin, "dependency", subcmd, chartDir, "--skip-refresh").CombinedOutput()
-	if err != nil {
-		st.logger.Debugf("execHelmDep: helm dependency %s failed for %s: %v\n%s", subcmd, chartDir, err, string(output))
-	} else {
-		st.logger.Debugf("execHelmDep: helm dependency %s succeeded for %s", subcmd, chartDir)
-	}
-	return output, err
-}
-
-// isLockOutOfSync returns true when helm's output indicates Chart.lock is
+// isLockOutOfSync returns true when helm's error/output indicates Chart.lock is
 // missing or stale — the signal to fall back from `build` to `update`.
-func isLockOutOfSync(output []byte) bool {
-	msg := string(output)
+func isLockOutOfSync(msg string) bool {
 	return strings.Contains(msg, "out of sync") ||
 		strings.Contains(msg, "lock file is out of date") ||
 		strings.Contains(msg, "no lock file") ||
@@ -1972,7 +1957,7 @@ func isLockOutOfSync(output []byte) bool {
 //
 // If exists, it will also patch resources by json patches, strategic-merge patches, and injectors.
 // processChartification handles the chartification process
-func (st *HelmState) processChartification(chartification *Chartify, release *ReleaseSpec, chartPath string, opts ChartPrepareOptions, skipDeps bool, helmfileCommand string) (string, bool, error) {
+func (st *HelmState) processChartification(chartification *Chartify, release *ReleaseSpec, chartPath string, helm helmexec.Interface, opts ChartPrepareOptions, skipDeps bool, helmfileCommand string) (string, bool, error) {
 	// Pre-build transitive local file:// subchart dependencies before chartify runs.
 	// helm's `dependency build` on the parent chart fetches each file:// subchart and
 	// packages it as .tgz, but does NOT recursively build the subchart's own deps.
@@ -1981,7 +1966,7 @@ func (st *HelmState) processChartification(chartification *Chartify, release *Re
 	// strategicMergePatches/jsonPatches targeting those resources fail with
 	// "no resource matches". See https://github.com/helmfile/helmfile/issues/851.
 	if !skipDeps && st.fs.DirectoryExistsAt(chartPath) {
-		st.preBuildTransitiveSubchartDeps(chartPath)
+		st.preBuildTransitiveSubchartDeps(chartPath, helm)
 	}

 	// Rewrite relative file:// dependencies in Chart.yaml to absolute paths before chartify processes them
@@ -2258,7 +2243,7 @@ func (st *HelmState) prepareChartForRelease(release *ReleaseSpec, helm helmexec.
 		if isLocal {
 			chartPath = normalizeChart(st.basePath, chartPath)
 		}
-		chartPath, buildDeps, err = st.processChartification(chartification, release, chartPath, opts, skipDeps, helmfileCommand)
+		chartPath, buildDeps, err = st.processChartification(chartification, release, chartPath, helm, opts, skipDeps, helmfileCommand)
 		if err != nil {
 			return &chartPrepareResult{err: err}
 		}