diff --git a/pkg/state/run_helm_dep_builds_skip_refresh_test.go b/pkg/state/run_helm_dep_builds_skip_refresh_test.go index 1975288f..cbe5a200 100644 --- a/pkg/state/run_helm_dep_builds_skip_refresh_test.go +++ b/pkg/state/run_helm_dep_builds_skip_refresh_test.go @@ -177,13 +177,13 @@ func TestRunHelmDepBuilds_SkipRefreshBehaviors(t *testing.T) { expectSkipRefreshFlag bool }{ { - name: "no skip flags and repos exist - UpdateRepo called, skip-refresh passed", + name: "local chart with repos exist - UpdateRepo skipped (all builds have skipRefresh=false), no skip-refresh flag (issue #2431)", optsSkipRefresh: false, helmDefaultsSkipRefresh: false, repos: []RepositorySpec{{Name: "stable", URL: "https://example.com"}}, precomputedSkipRefresh: false, - expectUpdateRepo: true, - expectSkipRefreshFlag: true, + expectUpdateRepo: false, + expectSkipRefreshFlag: false, }, { name: "opts.SkipRefresh=true - UpdateRepo skipped, skip-refresh flag preserved from precomputed value", @@ -231,7 +231,7 @@ func TestRunHelmDepBuilds_SkipRefreshBehaviors(t *testing.T) { expectSkipRefreshFlag: false, }, { - name: "mixed repos (OCI + non-OCI) - UpdateRepo called", + name: "mixed repos (OCI + non-OCI) with local chart - UpdateRepo skipped (all builds have skipRefresh=false), no skip-refresh flag", optsSkipRefresh: false, helmDefaultsSkipRefresh: false, repos: []RepositorySpec{ @@ -239,8 +239,17 @@ func TestRunHelmDepBuilds_SkipRefreshBehaviors(t *testing.T) { {Name: "stable", URL: "https://charts.helm.sh/stable"}, }, precomputedSkipRefresh: false, - expectUpdateRepo: true, - expectSkipRefreshFlag: true, + expectUpdateRepo: false, + expectSkipRefreshFlag: false, + }, + { + name: "build with skipRefresh=true and repos exist - UpdateRepo called, skip-refresh passed", + optsSkipRefresh: false, + helmDefaultsSkipRefresh: false, + repos: []RepositorySpec{{Name: "stable", URL: "https://example.com"}}, + precomputedSkipRefresh: true, + expectUpdateRepo: true, + expectSkipRefreshFlag: true, }, } @@ -305,9 +314,10 @@ func (h *multiBuildTracker) BuildDeps(name, chart string, flags ...string) error 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. +// TestRunHelmDepBuilds_MultipleBuilds verifies that when at least one build +// has skipRefresh=true, UpdateRepo is called and only builds with skipRefresh=true +// receive --skip-refresh. Builds with skipRefresh=false preserve their value to allow +// refreshing repos for external dependencies not in helmfile.yaml (issue #2431). func TestRunHelmDepBuilds_MultipleBuilds(t *testing.T) { helm := &multiBuildTracker{} @@ -332,6 +342,7 @@ func TestRunHelmDepBuilds_MultipleBuilds(t *testing.T) { assert.True(t, helm.updateRepoCalled, "UpdateRepo should have been called") assert.Len(t, helm.buildDepsCalls, 2, "BuildDeps should have been called twice") + expectedSkipRefresh := []bool{false, true} for i, flags := range helm.buildDepsCalls { hasSkipRefresh := false for _, f := range flags { @@ -340,7 +351,8 @@ func TestRunHelmDepBuilds_MultipleBuilds(t *testing.T) { break } } - assert.True(t, hasSkipRefresh, "build %d should have --skip-refresh flag (flags: %v)", i, flags) + assert.Equal(t, expectedSkipRefresh[i], hasSkipRefresh, + "build %d skip-refresh flag mismatch: expected %v, got %v (flags: %v)", i, expectedSkipRefresh[i], hasSkipRefresh, flags) } } diff --git a/pkg/state/state.go b/pkg/state/state.go index bb9f8359..b3a60c9b 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1857,29 +1857,29 @@ func (st *HelmState) runHelmDepBuilds(helm helmexec.Interface, concurrency int, // // See https://github.com/roboll/helmfile/issues/1521 - // Perform an update of repos once before running `helm dep build` so that we - // can safely pass --skip-refresh to the command to avoid doing a repo update - // 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() { + // Only run a global repo update when at least one build will actually use --skip-refresh. + // If all builds have skipRefresh=false, each `helm dep build` will refresh repos itself, + // so calling `helm.UpdateRepo()` here would be redundant. + anySkipRefresh := false + for _, b := range builds { + if b.skipRefresh { + anySkipRefresh = true + break + } + } + + if anySkipRefresh && !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 { - // 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 - } + // skipRefresh for each release is precomputed in prepareChartForRelease to: + // - avoid redundant repo updates for non-local charts when helm repo update has run + // - preserve refresh behavior for local charts that may depend on external repos + // not listed in helmfile.yaml (fixes issue #2431). + // We intentionally do not modify r.skipRefresh here. buildDepsFlags := getBuildDepsFlags(r) if err := helm.BuildDeps(r.releaseName, r.chartPath, buildDepsFlags...); err != nil { diff --git a/test/e2e/template/helmfile/testdata/snapshot/chart_need/output.yaml b/test/e2e/template/helmfile/testdata/snapshot/chart_need/output.yaml index 46577aa7..7954f3c1 100644 --- a/test/e2e/template/helmfile/testdata/snapshot/chart_need/output.yaml +++ b/test/e2e/template/helmfile/testdata/snapshot/chart_need/output.yaml @@ -1,12 +1,10 @@ Adding repo myrepo http://localhost:18080/ "myrepo" has been added to your repositories -Updating repo +Building dependency release=foo, chart=$WD/temp1/foo Hang tight while we grab the latest from your chart repositories... ...Successfully got an update from the "myrepo" chart repository Update Complete. ⎈Happy Helming!⎈ - -Building dependency release=foo, chart=$WD/temp1/foo Saving 1 charts Downloading raw from repo http://localhost:18080/ Deleting outdated charts diff --git a/test/e2e/template/helmfile/testdata/snapshot/chart_need_enable_live_output/output.yaml b/test/e2e/template/helmfile/testdata/snapshot/chart_need_enable_live_output/output.yaml index 61f4dd37..67616a32 100644 --- a/test/e2e/template/helmfile/testdata/snapshot/chart_need_enable_live_output/output.yaml +++ b/test/e2e/template/helmfile/testdata/snapshot/chart_need_enable_live_output/output.yaml @@ -2,12 +2,10 @@ Live output is enabled Adding repo myrepo http://localhost:18081/ "myrepo" has been added to your repositories -Updating repo +Building dependency release=foo, chart=$WD/temp1/foo Hang tight while we grab the latest from your chart repositories... ...Successfully got an update from the "myrepo" chart repository Update Complete. ⎈Happy Helming!⎈ - -Building dependency release=foo, chart=$WD/temp1/foo Saving 1 charts Downloading raw from repo http://localhost:18081/ Deleting outdated charts diff --git a/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/output.yaml b/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/output.yaml index 3cf214c1..8f4523d9 100644 --- a/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/output.yaml +++ b/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/output.yaml @@ -1,11 +1,6 @@ Adding repo myrepo http://localhost:18084/ "myrepo" has been added to your repositories -Updating repo -Hang tight while we grab the latest from your chart repositories... -...Successfully got an update from the "myrepo" chart repository -Update Complete. ⎈Happy Helming!⎈ - Building dependency release=foo1, chart=../../charts/raw-0.1.0 Building dependency release=foo2, chart=../../charts/raw-0.1.0 Templating release=foo1, chart=../../charts/raw-0.1.0 diff --git a/test/integration/run.sh b/test/integration/run.sh index bd2d93b6..195f5d26 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -134,6 +134,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes . ${dir}/test-cases/issue-2269.sh . ${dir}/test-cases/issue-2418.sh . ${dir}/test-cases/issue-2424-sequential-values-paths.sh +. ${dir}/test-cases/issue-2431.sh # ALL DONE ----------------------------------------------------------------------------------------------------------- diff --git a/test/integration/test-cases/issue-2431.sh b/test/integration/test-cases/issue-2431.sh new file mode 100644 index 00000000..ea44492d --- /dev/null +++ b/test/integration/test-cases/issue-2431.sh @@ -0,0 +1,41 @@ +#!/usr/bin/env bash + +# Test for issue #2431: Local chart with external dependencies +# +# helmfile.yaml has repos configured (vector), but NOT the repo that the +# local chart depends on (wiremind). The fix ensures that: +# 1. helm dep build does NOT receive --skip-refresh for local charts +# 2. helmfile template succeeds without "no cached repository" error +# +# This replicates the exact scenario from issue #2431. + +issue_2431_input_dir="${cases_dir}/issue-2431/input" +issue_2431_tmp=$(mktemp -d) +issue_2431_output="${issue_2431_tmp}/template.log" + +cleanup_issue_2431() { + rm -rf "${issue_2431_tmp}" +} +trap cleanup_issue_2431 EXIT + +test_start "issue-2431: Local chart with external dependency not in helmfile.yaml" + +info "Running helmfile template: local chart depends on wiremind repo (not configured in helmfile.yaml)" +${helmfile} -f "${issue_2431_input_dir}/helmfile.yaml" -e karma template > "${issue_2431_output}" 2>&1 || { + code=$? + cat "${issue_2431_output}" + + # Check if the failure is due to "no cached repository" or "no repository definition" error + if grep -q "no cached repository" "${issue_2431_output}" || grep -q "no repository definition" "${issue_2431_output}"; then + fail "Issue #2431 regression: helm dep build received --skip-refresh incorrectly for local chart" + fi + + fail "helmfile template failed with exit code ${code}" +} + +info "SUCCESS: helmfile template completed successfully" +info "Template output:" +cat "${issue_2431_output}" + +trap - EXIT +test_pass "issue-2431: Local chart with external dependency not in helmfile.yaml" diff --git a/test/integration/test-cases/issue-2431/input/helmfile.yaml b/test/integration/test-cases/issue-2431/input/helmfile.yaml new file mode 100644 index 00000000..b437a4f4 --- /dev/null +++ b/test/integration/test-cases/issue-2431/input/helmfile.yaml @@ -0,0 +1,28 @@ +# Issue #2431: Local chart with external dependencies +# +# helmfile.yaml has repos configured (vector), but NOT the repo that the chart +# depends on (wiremind). This tests that helm dep build does NOT receive +# --skip-refresh for local charts, allowing them to refresh repos for +# external dependencies not in helmfile.yaml. + +repositories: + ## Everything works if the wiremind repo is added here: + # - name: wiremind + # url: https://wiremind.github.io/wiremind-helm-charts + - name: vector + url: https://helm.vector.dev + +--- + +environments: + karma: + missingFileHandler: Debug + +--- + +releases: + - name: karma + namespace: default + chart: ./karma + createNamespace: false + missingFileHandler: "Info" diff --git a/test/integration/test-cases/issue-2431/input/karma/Chart.yaml b/test/integration/test-cases/issue-2431/input/karma/Chart.yaml new file mode 100644 index 00000000..0532bdde --- /dev/null +++ b/test/integration/test-cases/issue-2431/input/karma/Chart.yaml @@ -0,0 +1,10 @@ +apiVersion: v2 +name: karma +description: Local chart with external dependencies (issue #2431) +type: application +version: 1.0.0 +appVersion: "1.0.0" +dependencies: + - name: karma + version: 2.11.0 + repository: https://wiremind.github.io/wiremind-helm-charts diff --git a/test/integration/test-cases/issue-2431/input/karma/templates/cm.yaml b/test/integration/test-cases/issue-2431/input/karma/templates/cm.yaml new file mode 100644 index 00000000..8d1b8246 --- /dev/null +++ b/test/integration/test-cases/issue-2431/input/karma/templates/cm.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: karma-cm + namespace: {{ .Release.Namespace }} +data: + key: value