diff --git a/pkg/state/issue_2296_test.go b/pkg/state/run_helm_dep_builds_skip_refresh_test.go similarity index 71% rename from pkg/state/issue_2296_test.go rename to pkg/state/run_helm_dep_builds_skip_refresh_test.go index ebcce047..1975288f 100644 --- a/pkg/state/issue_2296_test.go +++ b/pkg/state/run_helm_dep_builds_skip_refresh_test.go @@ -139,6 +139,7 @@ version: 0.1.0 type updateRepoTracker struct { exectest.Helm updateRepoCalled bool + buildDepsFlags []string } func (h *updateRepoTracker) UpdateRepo() error { @@ -146,53 +147,88 @@ func (h *updateRepoTracker) UpdateRepo() error { return nil } -// TestRunHelmDepBuilds_HelmDefaultsSkipRefresh verifies that when -// helmDefaults.skipRefresh=true, runHelmDepBuilds skips the UpdateRepo call -// even when opts.SkipRefresh is false. This is a regression test for issue #2269. -// It also verifies that UpdateRepo is skipped when only OCI repos are configured, -// as OCI repos don't need `helm repo update` (they use `helm registry login` instead). -func TestRunHelmDepBuilds_HelmDefaultsSkipRefresh(t *testing.T) { +func (h *updateRepoTracker) BuildDeps(name, chart string, flags ...string) error { + h.buildDepsFlags = flags + return nil +} + +// TestRunHelmDepBuilds_SkipRefreshBehaviors verifies multiple behaviors around +// skipRefresh handling in runHelmDepBuilds: +// +// 1. When helmDefaults.skipRefresh=true, runHelmDepBuilds skips the UpdateRepo call +// even when opts.SkipRefresh is false (regression test for issue #2269). +// 2. When no repos are configured, helm dep build should not receive --skip-refresh +// so it can refresh repos for local charts with external dependencies +// (regression test for issue #2417). +// 3. UpdateRepo is skipped when only OCI repos are configured, +// as OCI repos don't need `helm repo update` (they use `helm registry login` instead). +// +// The precomputedSkipRefresh field simulates the skipRefresh value computed in +// prepareChartForRelease, which accounts for CLI flags, helmDefaults.skipRefresh, +// and release-level skipRefresh settings. +func TestRunHelmDepBuilds_SkipRefreshBehaviors(t *testing.T) { tests := []struct { name string optsSkipRefresh bool helmDefaultsSkipRefresh bool repos []RepositorySpec + precomputedSkipRefresh bool expectUpdateRepo bool + expectSkipRefreshFlag bool }{ { - name: "no skip flags and repos exist - UpdateRepo called", + name: "no skip flags and repos exist - UpdateRepo called, skip-refresh passed", optsSkipRefresh: false, helmDefaultsSkipRefresh: false, repos: []RepositorySpec{{Name: "stable", URL: "https://example.com"}}, + precomputedSkipRefresh: false, expectUpdateRepo: true, + expectSkipRefreshFlag: true, }, { - name: "opts.SkipRefresh=true - UpdateRepo skipped", + name: "opts.SkipRefresh=true - UpdateRepo skipped, skip-refresh flag preserved from precomputed value", optsSkipRefresh: true, helmDefaultsSkipRefresh: false, repos: []RepositorySpec{{Name: "stable", URL: "https://example.com"}}, + precomputedSkipRefresh: true, expectUpdateRepo: false, + expectSkipRefreshFlag: true, }, { - name: "helmDefaults.skipRefresh=true - UpdateRepo skipped", + name: "helmDefaults.skipRefresh=true - UpdateRepo skipped, skip-refresh flag preserved from precomputed value", optsSkipRefresh: false, helmDefaultsSkipRefresh: true, repos: []RepositorySpec{{Name: "stable", URL: "https://example.com"}}, + precomputedSkipRefresh: true, expectUpdateRepo: false, + expectSkipRefreshFlag: true, }, { - name: "no repos configured - UpdateRepo skipped", + name: "release-level skipRefresh=true - UpdateRepo called, skip-refresh flag preserved from precomputed value", + optsSkipRefresh: false, + helmDefaultsSkipRefresh: false, + repos: []RepositorySpec{{Name: "stable", URL: "https://example.com"}}, + precomputedSkipRefresh: true, + expectUpdateRepo: true, + expectSkipRefreshFlag: true, + }, + { + name: "no repos configured - UpdateRepo skipped, no skip-refresh flag (issue #2417)", optsSkipRefresh: false, helmDefaultsSkipRefresh: false, repos: nil, + precomputedSkipRefresh: false, expectUpdateRepo: false, + expectSkipRefreshFlag: false, }, { - name: "only OCI repos configured - UpdateRepo skipped", + name: "only OCI repos configured - UpdateRepo skipped, no skip-refresh flag", optsSkipRefresh: false, helmDefaultsSkipRefresh: false, repos: []RepositorySpec{{Name: "karpenter", URL: "public.ecr.aws/karpenter", OCI: true}}, + precomputedSkipRefresh: false, expectUpdateRepo: false, + expectSkipRefreshFlag: false, }, { name: "mixed repos (OCI + non-OCI) - UpdateRepo called", @@ -202,7 +238,9 @@ func TestRunHelmDepBuilds_HelmDefaultsSkipRefresh(t *testing.T) { {Name: "karpenter", URL: "public.ecr.aws/karpenter", OCI: true}, {Name: "stable", URL: "https://charts.helm.sh/stable"}, }, - expectUpdateRepo: true, + precomputedSkipRefresh: false, + expectUpdateRepo: true, + expectSkipRefreshFlag: true, }, } @@ -221,7 +259,7 @@ func TestRunHelmDepBuilds_HelmDefaultsSkipRefresh(t *testing.T) { } builds := []*chartPrepareResult{ - {releaseName: "test", chartPath: "/tmp/chart", buildDeps: true}, + {releaseName: "test", chartPath: "/tmp/chart", buildDeps: true, skipRefresh: tt.precomputedSkipRefresh}, } opts := ChartPrepareOptions{ @@ -231,12 +269,81 @@ func TestRunHelmDepBuilds_HelmDefaultsSkipRefresh(t *testing.T) { err := st.runHelmDepBuilds(helm, 1, builds, opts) require.NoError(t, err) + assert.NotNil(t, helm.buildDepsFlags, "BuildDeps should have been called") + assert.Equal(t, tt.expectUpdateRepo, helm.updateRepoCalled, "UpdateRepo called mismatch: expected %v, got %v", tt.expectUpdateRepo, helm.updateRepoCalled) + + hasSkipRefreshFlag := false + for _, f := range helm.buildDepsFlags { + if f == "--skip-refresh" { + hasSkipRefreshFlag = true + break + } + } + assert.Equal(t, tt.expectSkipRefreshFlag, hasSkipRefreshFlag, + "--skip-refresh flag mismatch: expected %v, got %v (flags: %v)", tt.expectSkipRefreshFlag, hasSkipRefreshFlag, helm.buildDepsFlags) }) } } +// multiBuildTracker tracks multiple BuildDeps calls for testing scenarios with +// multiple builds that have different skipRefresh values. +type multiBuildTracker struct { + exectest.Helm + updateRepoCalled bool + buildDepsCalls [][]string +} + +func (h *multiBuildTracker) UpdateRepo() error { + h.updateRepoCalled = true + return nil +} + +func (h *multiBuildTracker) BuildDeps(name, chart string, flags ...string) error { + h.buildDepsCalls = append(h.buildDepsCalls, flags) + return nil +} + +// TestRunHelmDepBuilds_MultipleBuilds verifies that when didUpdateRepo=true, +// all builds receive --skip-refresh regardless of their precomputed skipRefresh values. +// This ensures the override applies uniformly across all builds in the loop. +func TestRunHelmDepBuilds_MultipleBuilds(t *testing.T) { + helm := &multiBuildTracker{} + + st := &HelmState{ + logger: logger, + ReleaseSetSpec: ReleaseSetSpec{ + HelmDefaults: HelmSpec{SkipRefresh: false}, + Repositories: []RepositorySpec{{Name: "stable", URL: "https://example.com"}}, + }, + } + + builds := []*chartPrepareResult{ + {releaseName: "release-a", chartPath: "/tmp/chart-a", buildDeps: true, skipRefresh: false}, + {releaseName: "release-b", chartPath: "/tmp/chart-b", buildDeps: true, skipRefresh: true}, + } + + opts := ChartPrepareOptions{SkipRefresh: false} + + err := st.runHelmDepBuilds(helm, 1, builds, opts) + require.NoError(t, err) + + assert.True(t, helm.updateRepoCalled, "UpdateRepo should have been called") + assert.Len(t, helm.buildDepsCalls, 2, "BuildDeps should have been called twice") + + for i, flags := range helm.buildDepsCalls { + hasSkipRefresh := false + for _, f := range flags { + if f == "--skip-refresh" { + hasSkipRefresh = true + break + } + } + assert.True(t, hasSkipRefresh, "build %d should have --skip-refresh flag (flags: %v)", i, flags) + } +} + // TestSkipReposLogic tests the skipRepos calculation used in pkg/app/run.go. // This documents and verifies the expected behavior: both helmDefaults.skipDeps // and helmDefaults.skipRefresh should cause repo sync to be skipped. diff --git a/pkg/state/state.go b/pkg/state/state.go index 5b1c1ade..bb9f8359 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1862,16 +1862,24 @@ func (st *HelmState) runHelmDepBuilds(helm helmexec.Interface, concurrency int, // for every iteration of the loop where charts have external dependencies. // Only do this if there are non-OCI repositories configured. // OCI repositories don't need `helm repo update` as they use `helm registry login` instead. + didUpdateRepo := false if len(builds) > 0 && !opts.SkipRefresh && !st.HelmDefaults.SkipRefresh && st.NeedsRepoUpdate() { if err := helm.UpdateRepo(); err != nil { return fmt.Errorf("updating repo: %w", err) } + didUpdateRepo = true } for _, r := range builds { - // Never update the local repository cache here, since we've already - // updated it before entering the loop - r.skipRefresh = true + // If we ran helm repo update, pass --skip-refresh to helm dep build to avoid + // redundant repo updates. Otherwise, preserve the precomputed skipRefresh value + // which accounts for CLI flags, helmDefaults.skipRefresh, and release.skipRefresh. + // When no repos are configured in helmfile.yaml and user hasn't set skip-refresh, + // helm dep build will refresh repos as needed for local charts with external + // dependencies (fixes issue #2417). + if didUpdateRepo { + r.skipRefresh = true + } buildDepsFlags := getBuildDepsFlags(r) if err := helm.BuildDeps(r.releaseName, r.chartPath, buildDepsFlags...); err != nil {