fix: skip failed-prep releases, complete flag wiring, add tests and docs

Review follow-ups for --allow-failed-releases (#2616):

- Track per-release chart preparation failures in PrepareCharts (returned
  as a map keyed by release) and remove those releases from the state in
  Run.WithPreparedCharts when --allow-failed-releases is set, so a failed
  release is never executed against its original, un-prepared chart
  reference (which could either fail again with a duplicate error or, for
  charts requiring chartify, bypass patches/dependency modifications and
  produce an unintended result). All failures are still reported at the
  end via the aggregated MultiError.
- Complete the release identity on error results from
  prepareChartForRelease so failures are attributed to the correct
  release.
- With --allow-failed-releases, continue building dependencies of the
  remaining charts when 'helm dep build' fails for one release, and skip
  the affected releases during execution.
- Wire --allow-failed-releases into 'helmfile unittest' and 'helmfile
  status'; remove the dead flag wiring for write-values and list (both
  never prepare charts, see commandsSkipChartPrep).
- Simplify control flow (guard clauses, errors.As, drop dead code and
  redundant else branches).
- Add end-to-end coverage in pkg/app/issue_2616_test.go and extend the
  state-level tests; document the flag in docs/cli.md and CHANGELOG.md.

Signed-off-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

pkg/app/app.go

@@ -276,12 +276,13 @@ func (a *App) Template(c TemplateConfigProvider) error {
 func (a *App) WriteValues(c WriteValuesConfigProvider) error {
 	return a.ForEachState(func(run *Run) (ok bool, errs []error) {
 		prepErr := run.WithPreparedCharts("write-values", state.ChartPrepareOptions{
-			SkipRepos:           c.SkipRefresh() || c.SkipDeps(),
-			SkipRefresh:         c.SkipRefresh(),
-			AllowFailedReleases: c.AllowFailedReleases(),
-			SkipDeps:            c.SkipDeps(),
-			SkipCleanup:         c.SkipCleanup(),
-			Concurrency:         c.Concurrency(),
+			// Note: "write-values" never prepares charts (see commandsSkipChartPrep
+			// in run.go), so AllowFailedReleases does not apply here.
+			SkipRepos:   c.SkipRefresh() || c.SkipDeps(),
+			SkipRefresh: c.SkipRefresh(),
+			SkipDeps:    c.SkipDeps(),
+			SkipCleanup: c.SkipCleanup(),
+			Concurrency: c.Concurrency(),
 		}, func() []error {
 			ok, errs = a.writeValues(run, c)
 			return errs
@@ -375,6 +376,7 @@ func (a *App) Unittest(c UnittestConfigProvider) error {
 			ForceDownload:          true,
 			SkipRepos:              c.SkipRefresh() || c.SkipDeps(),
 			SkipRefresh:            c.SkipRefresh(),
+			AllowFailedReleases:    c.AllowFailedReleases(),
 			SkipDeps:               c.SkipDeps(),
 			SkipCleanup:            c.SkipCleanup(),
 			Concurrency:            c.Concurrency(),
@@ -615,9 +617,10 @@ func (a *App) Apply(c ApplyConfigProvider) error {
 func (a *App) Status(c StatusesConfigProvider) error {
 	return a.ForEachState(func(run *Run) (ok bool, errs []error) {
 		err := run.WithPreparedCharts("status", state.ChartPrepareOptions{
-			SkipRepos:   true,
-			SkipDeps:    true,
-			Concurrency: c.Concurrency(),
+			SkipRepos:           true,
+			AllowFailedReleases: c.AllowFailedReleases(),
+			SkipDeps:            true,
+			Concurrency:         c.Concurrency(),
 		}, func() []error {
 			ok, errs = a.status(run, c)
 			return errs
@@ -687,10 +690,9 @@ func (a *App) PrintDAGState(c DAGConfigProvider) error {
 	var err error
 	return a.ForEachState(func(run *Run) (ok bool, errs []error) {
 		err = run.WithPreparedCharts("show-dag", state.ChartPrepareOptions{
-			SkipRepos:           true,
-			AllowFailedReleases: false,
-			SkipDeps:            true,
-			Concurrency:         2,
+			SkipRepos:   true,
+			SkipDeps:    true,
+			Concurrency: 2,
 		}, func() []error {
 			err = a.dag(run)
 			if err != nil {
@@ -705,10 +707,9 @@ func (a *App) PrintDAGState(c DAGConfigProvider) error {
 func (a *App) PrintState(c StateConfigProvider) error {
 	return a.ForEachState(func(run *Run) (_ bool, errs []error) {
 		err := run.WithPreparedCharts("build", state.ChartPrepareOptions{
-			SkipRepos:           true,
-			AllowFailedReleases: false,
-			SkipDeps:            true,
-			Concurrency:         2,
+			SkipRepos:   true,
+			SkipDeps:    true,
+			Concurrency: 2,
 		}, func() []error {
 			if c.EmbedValues() {
 				for i := range run.state.Releases {
@@ -779,10 +780,11 @@ func (a *App) ListReleases(c ListConfigProvider) error {
 
 		if !c.SkipCharts() {
 			prepErr := run.WithPreparedCharts("list", state.ChartPrepareOptions{
-				SkipRepos:           true,
-				AllowFailedReleases: true,
-				SkipDeps:            true,
-				Concurrency:         2,
+				// Note: "list" never prepares charts (see commandsSkipChartPrep in
+				// run.go), so AllowFailedReleases does not apply here.
+				SkipRepos:   true,
+				SkipDeps:    true,
+				Concurrency: 2,
 			}, func() []error {
 				rel, err := a.list(run)
 				if err != nil {

pkg/app/config.go

@@ -343,7 +343,6 @@ type WriteValuesConfigProvider interface {
 	OutputFileTemplate() string
 	SkipDeps() bool
 	SkipRefresh() bool
-	AllowFailedReleases() bool
 	SkipCleanup() bool
 	IncludeTransitiveNeeds() bool
 
@@ -352,6 +351,7 @@ type WriteValuesConfigProvider interface {
 
 type StatusesConfigProvider interface {
 	Args() string
+	AllowFailedReleases() bool
 
 	concurrencyConfig
 }

pkg/app/issue_2616_test.go

@@ -0,0 +1,109 @@
+package app
+
+import (
+	"path/filepath"
+	"sync"
+	"testing"
+
+	"github.com/helmfile/vals"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/helmfile/helmfile/pkg/exectest"
+	ffs "github.com/helmfile/helmfile/pkg/filesystem"
+	"github.com/helmfile/helmfile/pkg/helmexec"
+)
+
+// issue2616HelmfileContent defines two releases: one that is templated fine
+// (logging) and one that always fails chart preparation (error), because OCI
+// charts do not support the "latest" version tag.
+const issue2616HelmfileContent = `
+releases:
+- name: logging
+  chart: incubator/raw
+  namespace: kube-system
+
+- name: error
+  chart: oci://example.com/chart/error
+  namespace: failed
+  version: latest
+`
+
+// TestTemplateAllowFailedReleases verifies the behavior of the
+// --allow-failed-releases flag end to end for `helmfile template`:
+// when chart preparation fails for one release, the remaining releases are
+// still templated, the failed release is skipped (i.e. never executed against
+// its un-prepared chart reference) and all failures are reported at the end.
+func TestTemplateAllowFailedReleases(t *testing.T) {
+	testcases := []struct {
+		name               string
+		allowFailedRelease bool
+
+		// wantTemplated contains the releases that must have been passed to
+		// `helm template`; the release failing chart preparation must never
+		// appear here.
+		wantTemplated []exectest.Release
+	}{
+		{
+			name:               "default: abort on chart preparation failure",
+			allowFailedRelease: false,
+			wantTemplated:      nil,
+		},
+		{
+			name:               "allow-failed-releases: skip failed release, template the rest",
+			allowFailedRelease: true,
+			wantTemplated:      []exectest.Release{{Name: "logging", Flags: []string{"--kube-context", "default", "--namespace", "kube-system"}}},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			var helm = &exectest.Helm{
+				FailOnUnexpectedList: true,
+				FailOnUnexpectedDiff: true,
+				DiffMutex:            &sync.Mutex{},
+				ChartsMutex:          &sync.Mutex{},
+				ReleasesMutex:        &sync.Mutex{},
+			}
+
+			_ = runWithLogCapture(t, "debug", func(t *testing.T, logger *zap.SugaredLogger) {
+				t.Helper()
+
+				valsRuntime, err := vals.New(vals.Options{CacheSize: 32})
+				require.NoError(t, err)
+
+				files := map[string]string{
+					"/path/to/helmfile.yaml": issue2616HelmfileContent,
+				}
+
+				app := appWithFs(&App{
+					OverrideHelmBinary:              DefaultHelmBinary,
+					fs:                              &ffs.FileSystem{Glob: filepath.Glob},
+					OverrideKubeContext:             "default",
+					DisableKubeVersionAutoDetection: true,
+					Env:                             "default",
+					Logger:                          logger,
+					helms: map[helmKey]helmexec.Interface{
+						createHelmKey("helm", "default"): helm,
+					},
+					valsRuntime: valsRuntime,
+				}, files)
+
+				tmplErr := app.Template(applyConfig{
+					// if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic.
+					concurrency:         1,
+					logger:              logger,
+					allowFailedReleases: tc.allowFailedRelease,
+				})
+
+				// The chart preparation failure must be reported in both modes.
+				require.Error(t, tmplErr)
+				assert.Contains(t, tmplErr.Error(), "the version for OCI charts should be semver compliant")
+
+				// The release that failed chart preparation must never be executed.
+				require.Equal(t, tc.wantTemplated, helm.Templated)
+			})
+		})
+	}
+}

pkg/app/run.go

@@ -1,6 +1,7 @@
 package app
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"slices"
@@ -48,23 +49,23 @@ func (r *Run) askForConfirmation(msg string) bool {
 // When skipRepos is false, SyncReposOnce still runs normally for all repos.
 var commandsSkipChartPrep = []string{"write-values", "list"}
 
-func (r *Run) prepareChartsIfNeeded(helmfileCommand string, dir string, concurrency int, opts state.ChartPrepareOptions) (map[state.PrepareChartKey]string, error) {
+func (r *Run) prepareChartsIfNeeded(helmfileCommand string, dir string, concurrency int, opts state.ChartPrepareOptions) (map[state.PrepareChartKey]string, map[state.PrepareChartKey]error, error) {
 	// Skip chart preparation for commands that don't need chart pulls
 	if slices.Contains(commandsSkipChartPrep, strings.ToLower(helmfileCommand)) {
-		return nil, nil
+		return nil, nil, nil
 	}
 
-	releaseToChart, errs := r.state.PrepareCharts(r.helm, dir, concurrency, helmfileCommand, opts)
+	releaseToChart, failedReleases, errs := r.state.PrepareCharts(r.helm, dir, concurrency, helmfileCommand, opts)
 	if len(errs) > 0 {
 		if !opts.AllowFailedReleases {
 			// abort on first error
-			return nil, fmt.Errorf("%v", errs)
+			return nil, nil, fmt.Errorf("%v", errs)
 		}
-		// return partial results with errors for the failed ones
-		return releaseToChart, &MultiError{Errors: errs}
+		// return partial results, along with the per-release errors for the failed ones
+		return releaseToChart, failedReleases, &MultiError{Errors: errs}
 	}
 
-	return releaseToChart, nil
+	return releaseToChart, failedReleases, nil
 }
 
 func (r *Run) WithPreparedCharts(helmfileCommand string, opts state.ChartPrepareOptions, f func() []error) error {
@@ -118,18 +119,25 @@ func (r *Run) WithPreparedCharts(helmfileCommand string, opts state.ChartPrepare
 	// remain available for the entire operation lifecycle. See issue #1799.
 	defer r.state.CleanupChartifyTempDirs()
 
-	releaseToChart, prepareErr := r.prepareChartsIfNeeded(helmfileCommand, dir, opts.Concurrency, opts)
-	// IMPORTANT: on opts.AllowFailedReleases: do not abort only on error here, just forward it to the caller in order to allow for partial results
-	// Only in case prepareCharts failed with a general error and returned to processable release abort anyways
-	if prepareErr != nil && releaseToChart == nil {
+	releaseToChart, failedReleases, prepareErr := r.prepareChartsIfNeeded(helmfileCommand, dir, opts.Concurrency, opts)
+	// A prepare error with no per-release attribution (state parsing, dependency
+	// resolution, ...) is a general failure: always abort, even with
+	// opts.AllowFailedReleases, since there are no processable partial results.
+	// Otherwise, abort only when partial results are not allowed; the failed
+	// releases are skipped below and their errors are reported at the end.
+	if prepareErr != nil && (len(failedReleases) == 0 || !opts.AllowFailedReleases) {
 		return prepareErr
 	}
-	if !opts.AllowFailedReleases {
-		if prepareErr != nil {
-			return prepareErr
-		}
-	}
 
+	// Releases whose chart preparation failed are removed from the state, so that
+	// the operation below never executes them against their original, un-prepared
+	// chart reference. That would either fail again with a duplicate error or,
+	// worse, bypass chartify modifications (patches, dependencies) and produce an
+	// unintended result. Their preparation errors are reported via prepareErr.
+	releases := r.state.Releases
+	if len(failedReleases) > 0 {
+		releases = make([]state.ReleaseSpec, 0, len(r.state.Releases))
+	}
 	for i := range r.state.Releases {
 		rel := &r.state.Releases[i]
 		key := state.PrepareChartKey{
@@ -137,6 +145,9 @@ func (r *Run) WithPreparedCharts(helmfileCommand string, opts state.ChartPrepare
 			Namespace:   rel.Namespace,
 			KubeContext: rel.KubeContext,
 		}
+		if _, failed := failedReleases[key]; failed {
+			continue
+		}
 		if chart, ok := releaseToChart[key]; ok && chart != rel.Chart {
 			// The chart has been downloaded and modified by Helmfile (and chartify under the hood).
 			// We let the later step use the modified version of the chart, located under the `chart` variable,
@@ -145,7 +156,11 @@ func (r *Run) WithPreparedCharts(helmfileCommand string, opts state.ChartPrepare
 			// if it has been modified or not.
 			rel.ChartPath = chart
 		}
+		if len(failedReleases) > 0 {
+			releases = append(releases, *rel)
+		}
 	}
+	r.state.Releases = releases
 
 	r.ReleaseToChart = releaseToChart
 
@@ -160,26 +175,26 @@ func (r *Run) WithPreparedCharts(helmfileCommand string, opts state.ChartPrepare
 
 	_, cleanupErr := r.state.TriggerGlobalCleanupEvent(helmfileCommand, firstErr)
 	if !opts.AllowFailedReleases {
-		// return directly on first error
 		return cleanupErr
-	} else {
-		// merge the two errors into a single error output
-		var merged []error
-		if prepareErr != nil {
-			if me, ok := prepareErr.(*MultiError); ok {
-				merged = append(merged, me.Errors...)
-			} else {
-				merged = append(merged, prepareErr)
-			}
-		}
-		if cleanupErr != nil {
-			merged = append(merged, fmt.Errorf("error during global cleanup event: %w", cleanupErr))
-		}
-		if len(merged) > 0 {
-			return &MultiError{Errors: merged}
-		}
-		return nil
 	}
+
+	// merge the preparation and cleanup errors into a single error output
+	var merged []error
+	if prepareErr != nil {
+		var me *MultiError
+		if errors.As(prepareErr, &me) {
+			merged = append(merged, me.Errors...)
+		} else {
+			merged = append(merged, prepareErr)
+		}
+	}
+	if cleanupErr != nil {
+		merged = append(merged, fmt.Errorf("error during global cleanup event: %w", cleanupErr))
+	}
+	if len(merged) > 0 {
+		return &MultiError{Errors: merged}
+	}
+	return nil
 }
 
 func (r *Run) Deps(c DepsConfigProvider) []error {