fix: cleanup hooks not receiving error signal (#2475)

* fix: cleanup hooks not receiving error signal Closes #1041 Signed-off-by: yxxhero <11087727+yxxhero@users.noreply.github.com> Signed-off-by: yxxhero <aiopsclub@163.com> * Add tests for cleanup hooks error propagation Signed-off-by: yxxhero <aiopsclub@163.com> * fix tests Signed-off-by: yxxhero <aiopsclub@163.com> * fix tests Signed-off-by: yxxhero <aiopsclub@163.com> * fix tests Signed-off-by: yxxhero <aiopsclub@163.com> * fix tests Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: yxxhero <11087727+yxxhero@users.noreply.github.com> Signed-off-by: yxxhero <aiopsclub@163.com>
2026-10-19 11:30:31 +02:00 · 2026-03-21 14:29:32 +08:00
parent 57c2b51eb0
commit 43b426892e
5 changed files with 300 additions and 27 deletions
@@ -170,8 +170,9 @@ func (a *App) Diff(c DiffConfigProvider) error {
 			Validate:               c.Validate(),
 			Concurrency:            c.Concurrency(),
 			IncludeTransitiveNeeds: c.IncludeNeeds(),
-		}, func() {
+		}, func() []error {
 			msg, matched, affected, errs = a.diff(run, c)
+			return errs
 		})

 		if msg != nil {
@@ -245,8 +246,9 @@ func (a *App) Template(c TemplateConfigProvider) error {
 			Values:                 c.Values(),
 			KubeVersion:            c.KubeVersion(),
 			HelmOCIPlainHTTP:       a.HelmOCIPlainHTTP,
-		}, func() {
+		}, func() []error {
 			ok, errs = a.template(run, c)
+			return errs
 		})

 		if prepErr != nil {
@@ -265,8 +267,9 @@ func (a *App) WriteValues(c WriteValuesConfigProvider) error {
 			SkipDeps:    c.SkipDeps(),
 			SkipCleanup: c.SkipCleanup(),
 			Concurrency: c.Concurrency(),
-		}, func() {
+		}, func() []error {
 			ok, errs = a.writeValues(run, c)
+			return errs
 		})

 		if prepErr != nil {
@@ -318,8 +321,9 @@ func (a *App) Lint(c LintConfigProvider) error {
 			SkipCleanup:            c.SkipCleanup(),
 			Concurrency:            c.Concurrency(),
 			IncludeTransitiveNeeds: c.IncludeNeeds(),
-		}, func() {
+		}, func() []error {
 			ok, lintErrs, errs = a.lint(run, c)
+			return append(errs, lintErrs...)
 		})

 		if prepErr != nil {
@@ -359,8 +363,9 @@ func (a *App) Unittest(c UnittestConfigProvider) error {
 			SkipCleanup:            c.SkipCleanup(),
 			Concurrency:            c.Concurrency(),
 			IncludeTransitiveNeeds: c.IncludeTransitiveNeeds(),
-		}, func() {
+		}, func() []error {
 			ok, unittestErrs, errs = a.unittest(run, c)
+			return append(errs, unittestErrs...)
 		})

 		if prepErr != nil {
@@ -395,7 +400,9 @@ func (a *App) Fetch(c FetchConfigProvider) error {
 			OutputDir:         c.OutputDir(),
 			OutputDirTemplate: c.OutputDirTemplate(),
 			Concurrency:       c.Concurrency(),
-		}, func() {})
+		}, func() []error {
+			return nil
+		})

 		if prepErr != nil {
 			errs = append(errs, prepErr)
@@ -420,8 +427,9 @@ func (a *App) Sync(c SyncConfigProvider) error {
 			IncludeTransitiveNeeds: c.IncludeNeeds(),
 			Validate:               c.Validate(),
 			Concurrency:            c.Concurrency(),
-		}, func() {
+		}, func() []error {
 			ok, errs = a.sync(run, c)
+			return errs
 		})

 		if prepErr != nil {
@@ -456,7 +464,7 @@ func (a *App) Apply(c ApplyConfigProvider) error {
 			Validate:               c.Validate(),
 			Concurrency:            c.Concurrency(),
 			IncludeTransitiveNeeds: c.IncludeNeeds(),
-		}, func() {
+		}, func() []error {
 			matched, updated, es := a.apply(run, c)

 			mut.Lock()
@@ -464,6 +472,7 @@ func (a *App) Apply(c ApplyConfigProvider) error {
 			mut.Unlock()

 			ok, errs = matched, es
+			return errs
 		})

 		if prepErr != nil {
@@ -492,8 +501,9 @@ func (a *App) Status(c StatusesConfigProvider) error {
 			SkipRepos:   true,
 			SkipDeps:    true,
 			Concurrency: c.Concurrency(),
-		}, func() {
+		}, func() []error {
 			ok, errs = a.status(run, c)
+			return errs
 		})

 		if err != nil {
@@ -514,8 +524,9 @@ func (a *App) Destroy(c DestroyConfigProvider) error {
 				Concurrency:   c.Concurrency(),
 				DeleteWait:    c.DeleteWait(),
 				DeleteTimeout: c.DeleteTimeout(),
-			}, func() {
+			}, func() []error {
 				ok, errs = a.delete(run, true, c)
+				return errs
 			})
 			if err != nil {
 				errs = append(errs, err)
@@ -540,8 +551,9 @@ func (a *App) Test(c TestConfigProvider) error {
 			SkipRefresh: c.SkipRefresh(),
 			SkipDeps:    c.SkipDeps(),
 			Concurrency: c.Concurrency(),
-		}, func() {
+		}, func() []error {
 			errs = a.test(run, c)
+			return errs
 		})

 		if err != nil {
@@ -559,11 +571,12 @@ func (a *App) PrintDAGState(c DAGConfigProvider) error {
 			SkipRepos:   true,
 			SkipDeps:    true,
 			Concurrency: 2,
-		}, func() {
+		}, func() []error {
 			err = a.dag(run)
 			if err != nil {
 				errs = append(errs, err)
 			}
+			return errs
 		})
 		return ok, errs
 	}, false, SetFilter(true))
@@ -575,7 +588,7 @@ func (a *App) PrintState(c StateConfigProvider) error {
 			SkipRepos:   true,
 			SkipDeps:    true,
 			Concurrency: 2,
-		}, func() {
+		}, func() []error {
 			if c.EmbedValues() {
 				for i := range run.state.Releases {
 					r := run.state.Releases[i]
@@ -583,7 +596,7 @@ func (a *App) PrintState(c StateConfigProvider) error {
 					values, err := run.state.LoadYAMLForEmbedding(&r, r.Values, r.MissingFileHandler, r.ValuesPathPrefix)
 					if err != nil {
 						errs = []error{err}
-						return
+						return errs
 					}

 					run.state.Releases[i].Values = values
@@ -591,7 +604,7 @@ func (a *App) PrintState(c StateConfigProvider) error {
 					secrets, err := run.state.LoadYAMLForEmbedding(&r, r.Secrets, r.MissingFileHandler, r.ValuesPathPrefix)
 					if err != nil {
 						errs = []error{err}
-						return
+						return errs
 					}

 					run.state.Releases[i].Secrets = secrets
@@ -601,17 +614,18 @@ func (a *App) PrintState(c StateConfigProvider) error {
 			stateYaml, err := run.state.ToYaml()
 			if err != nil {
 				errs = []error{err}
-				return
+				return errs
 			}

 			sourceFile, err := run.state.FullFilePath()
 			if err != nil {
 				errs = []error{err}
-				return
+				return errs
 			}
 			fmt.Printf("---\n#  Source: %s\n\n%+v", sourceFile, stateYaml)

 			errs = []error{}
+			return errs
 		})

 		if err != nil {
@@ -647,15 +661,18 @@ func (a *App) ListReleases(c ListConfigProvider) error {
 				SkipRepos:   true,
 				SkipDeps:    true,
 				Concurrency: 2,
-			}, func() {
-				stateReleases, listErr = a.list(run)
+			}, func() []error {
+				rel, err := a.list(run)
+				if err != nil {
+					errs = append(errs, err)
+					return []error{err}
+				}
+				stateReleases = rel
+				return nil
 			})
 			if prepErr != nil {
 				errs = append(errs, prepErr)
 			}
-			if listErr != nil {
-				errs = append(errs, listErr)
-			}
 		} else {
 			stateReleases, listErr = a.list(run)
 			if listErr != nil {
@@ -0,0 +1,121 @@
+package app
+
+import (
+	"sync"
+	"testing"
+
+	"github.com/helmfile/vals"
+	"github.com/stretchr/testify/assert"
+	"go.uber.org/zap"
+
+	"github.com/helmfile/helmfile/pkg/exectest"
+	ffs "github.com/helmfile/helmfile/pkg/filesystem"
+	"github.com/helmfile/helmfile/pkg/helmexec"
+)
+
+func TestCleanupHooksErrorPropagation(t *testing.T) {
+	type testcase struct {
+		files          map[string]string
+		releaseName    string
+		expectedError  bool
+		expectedInLogs string
+	}
+
+	check := func(t *testing.T, tc testcase) {
+		t.Helper()
+
+		var helm = &exectest.Helm{
+			FailOnUnexpectedList: true,
+			FailOnUnexpectedDiff: true,
+			DiffMutex:            &sync.Mutex{},
+			ChartsMutex:          &sync.Mutex{},
+			ReleasesMutex:        &sync.Mutex{},
+		}
+
+		valsRuntime, err := vals.New(vals.Options{CacheSize: 32})
+		if err != nil {
+			t.Fatalf("unexpected error creating vals runtime: %v", err)
+		}
+
+		bs := runWithLogCapture(t, "info", func(t *testing.T, logger *zap.SugaredLogger) {
+			t.Helper()
+
+			app := appWithFs(&App{
+				OverrideHelmBinary:              DefaultHelmBinary,
+				fs:                              ffs.DefaultFileSystem(),
+				OverrideKubeContext:             "default",
+				DisableKubeVersionAutoDetection: true,
+				Env:                             "default",
+				Logger:                          logger,
+				helms: map[helmKey]helmexec.Interface{
+					createHelmKey("helm", "default"): helm,
+				},
+				valsRuntime: valsRuntime,
+			}, tc.files)
+
+			syncErr := app.Sync(applyConfig{
+				concurrency: 1,
+				logger:      logger,
+			})
+
+			if tc.expectedError {
+				assert.Error(t, syncErr, "expected error for release %s", tc.releaseName)
+			} else {
+				assert.NoError(t, syncErr, "unexpected error for release %s", tc.releaseName)
+			}
+		})
+
+		logOutput := bs.String()
+		assert.Contains(t, logOutput, tc.expectedInLogs, "unexpected log output")
+	}
+
+	t.Run("cleanup hook receives error when sync fails", func(t *testing.T) {
+		check(t, testcase{
+			releaseName: "error-release",
+			files: map[string]string{
+				"/path/to/helmfile.yaml": `
+hooks:
+  - name: global-cleanup
+    events:
+      - cleanup
+    showlogs: true
+    command: echo
+    args:
+      - "error is '{{ .Event.Error }}'"
+
+releases:
+  - name: error-release
+    chart: incubator/raw
+    namespace: default
+`,
+			},
+			expectedError:  true,
+			expectedInLogs: "error is 'failed processing release error-release: error'",
+		})
+	})
+
+	t.Run("cleanup hook receives nil when sync succeeds", func(t *testing.T) {
+		check(t, testcase{
+			releaseName: "success-release",
+			files: map[string]string{
+				"/path/to/helmfile.yaml": `
+hooks:
+  - name: global-cleanup
+    events:
+      - cleanup
+    showlogs: true
+    command: echo
+    args:
+      - "error is '{{ .Event.Error }}'"
+
+releases:
+  - name: success-release
+    chart: incubator/raw
+    namespace: default
+`,
+			},
+			expectedError:  false,
+			expectedInLogs: "error is '<nil>'",
+		})
+	})
+}
@@ -57,7 +57,7 @@ func (r *Run) prepareChartsIfNeeded(helmfileCommand string, dir string, concurre
 	return releaseToChart, nil
 }

-func (r *Run) withPreparedCharts(helmfileCommand string, opts state.ChartPrepareOptions, f func()) error {
+func (r *Run) withPreparedCharts(helmfileCommand string, opts state.ChartPrepareOptions, f func() []error) error {
 	if r.ReleaseToChart != nil {
 		panic("Run.PrepareCharts can be called only once")
 	}
@@ -119,9 +119,16 @@ func (r *Run) withPreparedCharts(helmfileCommand string, opts state.ChartPrepare

 	r.ReleaseToChart = releaseToChart

-	f()
+	errs := f()
+	var firstErr error
+	for _, e := range errs {
+		if e != nil {
+			firstErr = e
+			break
+		}
+	}

-	_, err = r.state.TriggerGlobalCleanupEvent(helmfileCommand)
+	_, err = r.state.TriggerGlobalCleanupEvent(helmfileCommand, firstErr)
 	return err
 }