From 43b426892ef18b73ca217e2128e8566dd7275fec Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Sat, 21 Mar 2026 14:29:32 +0800 Subject: [PATCH] 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 * Add tests for cleanup hooks error propagation Signed-off-by: yxxhero * fix tests Signed-off-by: yxxhero * fix tests Signed-off-by: yxxhero * fix tests Signed-off-by: yxxhero * fix tests Signed-off-by: yxxhero --------- Signed-off-by: yxxhero <11087727+yxxhero@users.noreply.github.com> Signed-off-by: yxxhero --- pkg/app/app.go | 61 ++++++++----- pkg/app/cleanup_hooks_error_test.go | 121 ++++++++++++++++++++++++++ pkg/app/run.go | 13 ++- pkg/event/bus_test.go | 128 ++++++++++++++++++++++++++++ pkg/state/state.go | 4 +- 5 files changed, 300 insertions(+), 27 deletions(-) create mode 100644 pkg/app/cleanup_hooks_error_test.go diff --git a/pkg/app/app.go b/pkg/app/app.go index a93ced24..7fb6da5e 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -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 { diff --git a/pkg/app/cleanup_hooks_error_test.go b/pkg/app/cleanup_hooks_error_test.go new file mode 100644 index 00000000..9a877486 --- /dev/null +++ b/pkg/app/cleanup_hooks_error_test.go @@ -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 ''", + }) + }) +} diff --git a/pkg/app/run.go b/pkg/app/run.go index 10bd281d..91e25bb5 100644 --- a/pkg/app/run.go +++ b/pkg/app/run.go @@ -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 } diff --git a/pkg/event/bus_test.go b/pkg/event/bus_test.go index feb4eb2a..26c4b0f3 100644 --- a/pkg/event/bus_test.go +++ b/pkg/event/bus_test.go @@ -1,8 +1,10 @@ package event import ( + "errors" "fmt" "io" + "strings" "testing" "go.uber.org/zap" @@ -13,6 +15,11 @@ import ( ) type runner struct { + executeCalls []struct { + cmd string + args []string + env map[string]string + } } func (r *runner) ExecuteStdIn(cmd string, args []string, env map[string]string, stdin io.Reader) ([]byte, error) { @@ -20,6 +27,11 @@ func (r *runner) ExecuteStdIn(cmd string, args []string, env map[string]string, } func (r *runner) Execute(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + r.executeCalls = append(r.executeCalls, struct { + cmd string + args []string + env map[string]string + }{cmd: cmd, args: args, env: env}) if cmd == "ng" { return nil, fmt.Errorf("cmd failed due to invalid cmd: %s", cmd) } @@ -188,3 +200,119 @@ func TestTrigger(t *testing.T) { } } } + +func TestTriggerCleanupEventWithError(t *testing.T) { + runner := &runner{} + + core, _ := observer.New(zap.InfoLevel) + logger := zap.New(core).Sugar() + + testError := errors.New("sync failed: release error") + + hooks := []Hook{ + { + Name: "cleanup-with-error", + Events: []string{"cleanup"}, + Command: "echo", + Args: []string{"error is '{{ .Event.Error }}'"}, + ShowLogs: true, + }, + } + + bus := &Bus{ + Hooks: hooks, + StateFilePath: "/path/to/helmfile.yaml", + BasePath: ".", + Namespace: "default", + Env: environment.Environment{Name: "default"}, + Logger: logger, + Fs: ffs.DefaultFileSystem(), + Runner: runner, + } + + data := map[string]any{ + "HelmfileCommand": "sync", + } + + executed, err := bus.Trigger("cleanup", testError, data) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !executed { + t.Fatal("expected cleanup hook to be executed") + } + + if len(runner.executeCalls) != 1 { + t.Fatalf("expected 1 execute call, got %d", len(runner.executeCalls)) + } + + call := runner.executeCalls[0] + if call.cmd != "echo" { + t.Errorf("expected command 'echo', got %q", call.cmd) + } + + if len(call.args) != 1 { + t.Fatalf("expected 1 arg, got %d", len(call.args)) + } + + expectedArg := "error is 'sync failed: release error'" + if !strings.Contains(call.args[0], "error is") { + t.Errorf("expected arg to contain 'error is', got %q", call.args[0]) + } + + if call.args[0] != expectedArg { + t.Errorf("expected arg %q, got %q", expectedArg, call.args[0]) + } +} + +func TestTriggerCleanupEventWithNilError(t *testing.T) { + runner := &runner{} + + core, _ := observer.New(zap.InfoLevel) + logger := zap.New(core).Sugar() + + hooks := []Hook{ + { + Name: "cleanup-nil-error", + Events: []string{"cleanup"}, + Command: "echo", + Args: []string{"error is '{{ .Event.Error }}'"}, + ShowLogs: true, + }, + } + + bus := &Bus{ + Hooks: hooks, + StateFilePath: "/path/to/helmfile.yaml", + BasePath: ".", + Namespace: "default", + Env: environment.Environment{Name: "default"}, + Logger: logger, + Fs: ffs.DefaultFileSystem(), + Runner: runner, + } + + data := map[string]any{ + "HelmfileCommand": "sync", + } + + executed, err := bus.Trigger("cleanup", nil, data) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !executed { + t.Fatal("expected cleanup hook to be executed") + } + + if len(runner.executeCalls) != 1 { + t.Fatalf("expected 1 execute call, got %d", len(runner.executeCalls)) + } + + call := runner.executeCalls[0] + expectedArg := "error is ''" + if call.args[0] != expectedArg { + t.Errorf("expected arg %q, got %q", expectedArg, call.args[0]) + } +} diff --git a/pkg/state/state.go b/pkg/state/state.go index 5f929fd5..56e4a361 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -3105,8 +3105,8 @@ func (st *HelmState) TriggerGlobalPrepareEvent(helmfileCommand string) (bool, er return st.triggerGlobalReleaseEvent("prepare", nil, helmfileCommand) } -func (st *HelmState) TriggerGlobalCleanupEvent(helmfileCommand string) (bool, error) { - return st.triggerGlobalReleaseEvent("cleanup", nil, helmfileCommand) +func (st *HelmState) TriggerGlobalCleanupEvent(helmfileCommand string, evtErr error) (bool, error) { + return st.triggerGlobalReleaseEvent("cleanup", evtErr, helmfileCommand) } func (st *HelmState) triggerGlobalReleaseEvent(evt string, evtErr error, helmfileCmd string) (bool, error) {