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 <aiopsclub@163.com>

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) {

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
 }

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

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")
+}

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.