From bfcabf3eadfaf61e9bf869bc7b06526fb27093f1 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Wed, 1 Jul 2026 21:25:03 +0800 Subject: [PATCH] fix: address Copilot review comments on preBuildTransitiveSubchartDeps - Correct parseLocalFileDeps doc to match behavior (ok=false only when Chart.yaml is missing/unreadable/unparseable; may return empty slice). - Align log prefixes with function names (parseLocalFileDeps, runHelmDepBuild, execHelmDep) instead of the stale preBuildSubchartDeps. - Log execHelmDep failures at Debug (best-effort) instead of Warn. - Add skipIfNoHelm helper and guard tests that invoke helm. - Rename NoOpOnRemoteDeps -> DoesNotRecurseIntoRemoteDeps for accuracy. Signed-off-by: yxxhero --- pkg/state/prebuild_subchart_deps_test.go | 21 ++++++++++++++++++--- pkg/state/state.go | 11 ++++++----- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/pkg/state/prebuild_subchart_deps_test.go b/pkg/state/prebuild_subchart_deps_test.go index 59cfa2ac..cfb1a245 100644 --- a/pkg/state/prebuild_subchart_deps_test.go +++ b/pkg/state/prebuild_subchart_deps_test.go @@ -2,6 +2,7 @@ package state import ( "os" + "os/exec" "path/filepath" "strings" "testing" @@ -12,6 +13,16 @@ import ( "github.com/helmfile/helmfile/pkg/filesystem" ) +// 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. +func skipIfNoHelm(t *testing.T) { + t.Helper() + if _, err := exec.LookPath("helm"); err != nil { + t.Skipf("skipping: helm binary not found in PATH (%v)", err) + } +} + // 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) { @@ -53,6 +64,7 @@ func TestPreBuildTransitiveSubchartDeps_VisitsAllTransitiveDeps(t *testing.T) { if testing.Short() { t.Skip("skipping in short mode (requires helm)") } + skipIfNoHelm(t) rootDir, err := os.MkdirTemp("", "helmfile-prebuild-") if err != nil { @@ -152,6 +164,7 @@ func TestPreBuildTransitiveSubchartDeps_HandlesCircularDeps(t *testing.T) { if testing.Short() { t.Skip("skipping in short mode (requires helm)") } + skipIfNoHelm(t) rootDir, err := os.MkdirTemp("", "helmfile-prebuild-cycle-") if err != nil { @@ -187,9 +200,11 @@ func TestPreBuildTransitiveSubchartDeps_HandlesCircularDeps(t *testing.T) { } } -// TestPreBuildTransitiveSubchartDeps_NoOpOnRemoteDeps verifies the function -// skips dependencies declared with non-file:// repositories (https://, oci://). -func TestPreBuildTransitiveSubchartDeps_NoOpOnRemoteDeps(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. +func TestPreBuildTransitiveSubchartDeps_DoesNotRecurseIntoRemoteDeps(t *testing.T) { rootDir, err := os.MkdirTemp("", "helmfile-prebuild-remote-") if err != nil { t.Fatalf("mkdtemp: %v", err) diff --git a/pkg/state/state.go b/pkg/state/state.go index 9578406a..d1bb0983 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1871,7 +1871,8 @@ func (st *HelmState) prebuildChartDeps(chartDir string, visited map[string]bool) // parseLocalFileDeps reads chartDir/Chart.yaml and returns the absolute paths // of all local file:// dependencies that exist as directories. Returns ok=false -// if Chart.yaml is missing, unreadable, or has no file:// deps. +// only when Chart.yaml is missing, unreadable, or unparseable. When ok=true the +// returned slice may be empty (e.g. no file:// deps present). func (st *HelmState) parseLocalFileDeps(chartDir string) (depDirs []string, ok bool) { data, err := st.fs.ReadFile(filepath.Join(chartDir, "Chart.yaml")) if err != nil { @@ -1886,7 +1887,7 @@ func (st *HelmState) parseLocalFileDeps(chartDir string) (depDirs []string, ok b } var meta chartMeta if err := yaml.Unmarshal(data, &meta); err != nil { - st.logger.Debugf("preBuildSubchartDeps: failed to parse %s/Chart.yaml: %v", chartDir, err) + st.logger.Debugf("parseLocalFileDeps: failed to parse %s/Chart.yaml: %v", chartDir, err) return nil, false } @@ -1940,7 +1941,7 @@ func (st *HelmState) runHelmDepBuild(chartDir string) { // (re-resolves from Chart.yaml) when the lock is missing or stale. output, err := st.execHelmDep(helmBin, "build", chartDir) if err != nil && isLockOutOfSync(output) { - st.logger.Debugf("preBuildSubchartDeps: falling back to `dependency update` for %s", chartDir) + st.logger.Debugf("runHelmDepBuild: falling back to `dependency update` for %s", chartDir) _, _ = st.execHelmDep(helmBin, "update", chartDir) } } @@ -1950,9 +1951,9 @@ func (st *HelmState) runHelmDepBuild(chartDir string) { 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.Warnf("preBuildSubchartDeps: helm dependency %s failed for %s: %v\n%s", subcmd, chartDir, err, string(output)) + st.logger.Debugf("execHelmDep: helm dependency %s failed for %s: %v\n%s", subcmd, chartDir, err, string(output)) } else { - st.logger.Debugf("preBuildSubchartDeps: helm dependency %s succeeded for %s", subcmd, chartDir) + st.logger.Debugf("execHelmDep: helm dependency %s succeeded for %s", subcmd, chartDir) } return output, err }