From 08d70bc9a7f767398355c7dc6cab02fb42bb046f Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Thu, 25 Jun 2026 14:36:50 +0800 Subject: [PATCH] fix: ensure OCI charts are prepared for needed releases with --include-needs (#2663) fix: ensure OCI charts are prepared for needed releases with --include-needs (#923) When using --include-needs with a selector, releases included via needs must have their charts prepared (pulled/exported) before diff/sync/apply can process them. The core fix (ChartPrepareOptions.IncludeTransitiveNeeds = c.IncludeNeeds()) was already in place, but ForEachState calls in Diff/Template/Lint/Unittest/Sync/Apply still passed c.IncludeTransitiveNeeds() instead of c.IncludeNeeds(), creating an inconsistency that would resurface if SetFilter(true) were ever added. Changes: - Use c.IncludeNeeds() in ForEachState for all commands supporting --include-needs (Diff, Template, Lint, Unittest, Sync, Apply, Doctor) - Doctor is the only command with SetFilter(true), so this fixes a real bug: helmfile doctor --include-needs was silently ignored for filtering - Add explanatory doc comment on ForEachState parameter semantics - Enhance exectest.Helm.ChartPull to create minimal chart files and track pulls, enabling OCI chart testing - Add resetChartCacheForTest() for test isolation from global chart cache - Add regression tests for issue #923 Signed-off-by: yxxhero <11087727+yxxhero@users.noreply.github.com> Signed-off-by: yxxhero --- pkg/app/app.go | 20 ++-- pkg/app/doctor.go | 2 +- pkg/exectest/helm.go | 15 ++- pkg/state/issue923_test.go | 200 +++++++++++++++++++++++++++++++++++++ pkg/state/state.go | 7 ++ 5 files changed, 235 insertions(+), 9 deletions(-) create mode 100644 pkg/state/issue923_test.go diff --git a/pkg/app/app.go b/pkg/app/app.go index e2c0b0f9..ade46a0a 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -208,7 +208,7 @@ func (a *App) Diff(c DiffConfigProvider) error { } return matched, criticalErrs - }, c.IncludeTransitiveNeeds()) + }, c.IncludeNeeds()) if err != nil { return err @@ -264,7 +264,7 @@ func (a *App) Template(c TemplateConfigProvider) error { } return - }, c.IncludeTransitiveNeeds()) + }, c.IncludeNeeds()) } func (a *App) WriteValues(c WriteValuesConfigProvider) error { @@ -343,7 +343,7 @@ func (a *App) Lint(c LintConfigProvider) error { } return - }, c.IncludeTransitiveNeeds()) + }, c.IncludeNeeds()) if err != nil { return err @@ -385,7 +385,7 @@ func (a *App) Unittest(c UnittestConfigProvider) error { } return - }, c.IncludeTransitiveNeeds()) + }, c.IncludeNeeds()) if err != nil { return err @@ -526,7 +526,7 @@ func (a *App) Sync(c SyncConfigProvider) error { } return - }, c.IncludeTransitiveNeeds()) + }, c.IncludeNeeds()) if err != nil { return err @@ -582,7 +582,7 @@ func (a *App) Apply(c ApplyConfigProvider) error { } return - }, c.IncludeTransitiveNeeds(), opts...) + }, c.IncludeNeeds(), opts...) if err != nil { return err @@ -1356,6 +1356,14 @@ var ( } ) +// ForEachState iterates over each loaded state file and invokes do. +// +// includeTransitiveNeeds controls whether releases reachable via "needs" are +// un-filtered when SetFilter(true) is used. Despite the name, passing true +// unmarks ALL needs (both direct and transitive) via collectNeedsWithTransitives. +// Callers that support --include-needs should pass c.IncludeNeeds() (which is +// true for both --include-needs and --include-transitive-needs) so that chart +// preparation and early filtering stay consistent. See issue #923. func (a *App) ForEachState(do func(*Run) (bool, []error), includeTransitiveNeeds bool, o ...LoadOption) error { ctx := NewContext() err := a.visitStatesWithSelectorsAndRemoteSupportWithContext(a.FileOrDir, func(st *state.HelmState) (bool, []error) { diff --git a/pkg/app/doctor.go b/pkg/app/doctor.go index fbe55138..19dd9c1f 100644 --- a/pkg/app/doctor.go +++ b/pkg/app/doctor.go @@ -196,7 +196,7 @@ func (a *App) peekDoctorContext(c DoctorConfigProvider) (llm.Config, []string, e } } return false, nil - }, c.IncludeTransitiveNeeds(), SetFilter(true)) + }, c.IncludeNeeds(), SetFilter(true)) return yamlLLM, releases, err } diff --git a/pkg/exectest/helm.go b/pkg/exectest/helm.go index 2531f6f1..2c1ddeff 100644 --- a/pkg/exectest/helm.go +++ b/pkg/exectest/helm.go @@ -6,6 +6,7 @@ import ( "io" "os" "os/exec" + "path/filepath" "strings" "sync" @@ -33,7 +34,8 @@ type DiffKey struct { type Helm struct { Charts []string Repo []string - RegistryLoginHost string // Captures the host passed to RegistryLogin + RegistryLoginHost string // Captures the host passed to RegistryLogin + PulledCharts []string // Captures the OCI chart refs passed to ChartPull Releases []Release Deleted []Release Linted []Release @@ -233,7 +235,16 @@ func (helm *Helm) TemplateRelease(name, chart string, flags ...string) error { return nil } func (helm *Helm) ChartPull(chart string, path string, flags ...string) error { - return nil + helm.sync(helm.ChartsMutex, func() { + helm.PulledCharts = append(helm.PulledCharts, chart) + }) + // Create a minimal chart structure so getOCIChart's findChartDirectory succeeds + chartDir := filepath.Join(path, "chart") + if err := os.MkdirAll(chartDir, 0755); err != nil { + return err + } + chartYaml := "apiVersion: v2\nname: test-chart\ndescription: A test chart\ntype: application\nversion: 0.1.0\nappVersion: \"1.0.0\"\n" + return os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), []byte(chartYaml), 0644) } func (helm *Helm) ChartExport(chart string, path string) error { return nil diff --git a/pkg/state/issue923_test.go b/pkg/state/issue923_test.go new file mode 100644 index 00000000..6635eb49 --- /dev/null +++ b/pkg/state/issue923_test.go @@ -0,0 +1,200 @@ +package state + +import ( + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/helmfile/helmfile/pkg/exectest" +) + +// TestIssue923_OCIChartPreparedForNeededRelease verifies that OCI charts are +// prepared (pulled) for releases that are included via --include-needs. +// See https://github.com/helmfile/helmfile/issues/923 +func TestIssue923_OCIChartPreparedForNeededRelease(t *testing.T) { + resetChartCacheForTest() + helmfileContent := []byte(` +repositories: + - name: huma + url: ghcr.io/huma-engineering/helm-charts + oci: true + - name: argo + url: https://argoproj.github.io/argo-helm + +releases: + - name: argocd + namespace: argocd + chart: argo/argo-cd + labels: + app: argocd + needs: + - argocd/argocd-secrets + version: ~5.37.1 + + - name: argocd-secrets + namespace: argocd + chart: huma/external-secret-resources + labels: + app: secrets + version: ~0.1.0 +`) + + logger := zap.NewExample().Sugar() + + st, err := createFromYaml(helmfileContent, "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + // Simulate: helmfile -l app=argocd --include-needs + st.Selectors = []string{"app=argocd"} + + tempDir := t.TempDir() + + helm := &exectest.Helm{ + Helm3: true, + ChartsMutex: &sync.Mutex{}, + } + + // PrepareCharts uses opts.IncludeTransitiveNeeds to determine which releases + // to prepare charts for. When --include-needs is set, this should be true, + // matching c.IncludeNeeds() in the app layer. + // OutputDirTemplate is set to force charts into tempDir (not the global cache). + opts := ChartPrepareOptions{ + SkipResolve: true, + IncludeTransitiveNeeds: true, + Concurrency: 1, + OutputDirTemplate: "{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(helm, tempDir, 1, "apply", opts) + require.Empty(t, errs, "PrepareCharts should not return errors") + + // Verify both releases have prepared charts + assert.Contains(t, releaseToChart, PrepareChartKey{Name: "argocd", Namespace: "argocd"}, + "argocd chart should be prepared") + assert.Contains(t, releaseToChart, PrepareChartKey{Name: "argocd-secrets", Namespace: "argocd"}, + "argocd-secrets (needed OCI chart) should be prepared") + + // The OCI chart path should be a local path (pulled by ChartPull), not the + // remote chart name. This confirms the OCI chart was actually prepared. + chartPath := releaseToChart[PrepareChartKey{Name: "argocd-secrets", Namespace: "argocd"}] + assert.NotEqual(t, "huma/external-secret-resources", chartPath, + "argocd-secrets chart path should be a local path (pulled), not the remote chart name") +} + +// TestIssue923_OCIChartNotPreparedWithoutIncludeNeeds verifies that without +// --include-needs, the OCI chart for the needed release is NOT prepared. +func TestIssue923_OCIChartNotPreparedWithoutIncludeNeeds(t *testing.T) { + resetChartCacheForTest() + helmfileContent := []byte(` +repositories: + - name: huma + url: ghcr.io/huma-engineering/helm-charts + oci: true + +releases: + - name: argocd + namespace: argocd + chart: argo/argo-cd + labels: + app: argocd + needs: + - argocd/argocd-secrets + + - name: argocd-secrets + namespace: argocd + chart: huma/external-secret-resources + labels: + app: secrets + version: ~0.1.0 +`) + + logger := zap.NewExample().Sugar() + + st, err := createFromYaml(helmfileContent, "example/path/to/helmfile.yaml", DefaultEnv, logger) + require.NoError(t, err) + + // Simulate: helmfile -l app=argocd (without --include-needs) + st.Selectors = []string{"app=argocd"} + + tempDir := t.TempDir() + + helm := &exectest.Helm{ + Helm3: true, + ChartsMutex: &sync.Mutex{}, + } + + opts := ChartPrepareOptions{ + SkipResolve: true, + IncludeTransitiveNeeds: false, + Concurrency: 1, + OutputDirTemplate: "{{ .Release.Name }}", + } + + releaseToChart, errs := st.PrepareCharts(helm, tempDir, 1, "apply", opts) + require.Empty(t, errs) + + // argocd-secrets should NOT have a prepared chart + assert.NotContains(t, releaseToChart, PrepareChartKey{Name: "argocd-secrets", Namespace: "argocd"}, + "argocd-secrets chart should NOT be prepared without --include-needs") + assert.Empty(t, helm.PulledCharts, + "ChartPull should NOT have been called without --include-needs") +} + +// TestIssue923_GetSelectedReleasesWithNeeds verifies the selection logic +// that PrepareCharts depends on. +func TestIssue923_GetSelectedReleasesWithNeeds(t *testing.T) { + helmfileContent := []byte(` +repositories: + - name: huma + url: ghcr.io/huma-engineering/helm-charts + oci: true + +releases: + - name: argocd + namespace: argocd + chart: argo/argo-cd + labels: + app: argocd + needs: + - argocd/argocd-secrets + + - name: argocd-secrets + namespace: argocd + chart: huma/external-secret-resources + labels: + app: secrets +`) + + logger := zap.NewExample().Sugar() + st, err := createFromYaml(helmfileContent, filepath.Join("example", "path", "to", "helmfile.yaml"), DefaultEnv, logger) + require.NoError(t, err) + + st.Selectors = []string{"app=argocd"} + + // With includeTransitiveNeeds=true (as PrepareCharts does when --include-needs is set) + selected, err := st.GetSelectedReleases(true) + require.NoError(t, err) + + names := make([]string, len(selected)) + for i, r := range selected { + names[i] = r.Name + } + assert.Contains(t, names, "argocd", "selected releases should include argocd") + assert.Contains(t, names, "argocd-secrets", "selected releases should include the needed release argocd-secrets") + + // With includeTransitiveNeeds=false (without --include-needs) + selectedFalse, err := st.GetSelectedReleases(false) + require.NoError(t, err) + + namesFalse := make([]string, len(selectedFalse)) + for i, r := range selectedFalse { + namesFalse[i] = r.Name + } + assert.Contains(t, namesFalse, "argocd") + assert.NotContains(t, namesFalse, "argocd-secrets", + "needed release should NOT be selected without --include-needs") +} diff --git a/pkg/state/state.go b/pkg/state/state.go index 9c1230e2..0e118ee9 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -5356,6 +5356,13 @@ func (st *HelmState) addToChartCache(key ChartCacheKey, path string) { downloadedCharts[key] = path } +// resetChartCacheForTest clears the global chart cache. For testing only. +func resetChartCacheForTest() { + downloadedChartsMutex.Lock() + defer downloadedChartsMutex.Unlock() + downloadedCharts = make(map[ChartCacheKey]string) +} + // isSharedCachePath returns true if the chartPath is within the shared cache directory. // Charts in the shared cache should not be deleted during refresh to prevent race conditions // when multiple processes are using the same cached chart.