From 7a557580e1b5aac8327331e720e74bbbe49c0ad7 Mon Sep 17 00:00:00 2001 From: Michelle Fernandez Bieber Date: Sat, 22 Jan 2022 23:20:18 +0100 Subject: [PATCH 1/8] added shutdown hook that is executed after the logs have been retrieved Signed-off-by: Michelle Fernandez Bieber --- pkg/action/hooks.go | 75 ++++++++++++++++++++++------------- pkg/action/release_testing.go | 20 +++++----- pkg/cmd/release_testing.go | 3 +- 3 files changed, 61 insertions(+), 37 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 1e4fec9bd..c86964c65 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -33,6 +33,24 @@ import ( // execHook executes all of the hooks for the given hook event. func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) error { + shutdown, err := cfg.execHookWithDelayedShutdown(rl, hook, waitStrategy, timeout, serverSideApply) + if err != nil { + if err := shutdown(); err != nil { + return err + } + return err + } + return shutdown() +} + +type ExecuteShutdownHooks = func() error + +func ShutdownNoOp() error { + return nil +} + +// execHook executes all of the hooks for the given hook event and return a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs. +func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (ExecuteShutdownHooks, error) { executingHooks := []*release.Hook{} for _, h := range rl.Hooks { @@ -51,12 +69,12 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, cfg.hookSetDeletePolicy(h) if err := cfg.deleteHookByPolicy(h, release.HookBeforeHookCreation, waitStrategy, timeout); err != nil { - return err + return ShutdownNoOp, err } resources, err := cfg.KubeClient.Build(bytes.NewBufferString(h.Manifest), true) if err != nil { - return fmt.Errorf("unable to build kubernetes object for %s hook %s: %w", hook, h.Path, err) + return ShutdownNoOp, fmt.Errorf("unable to build kubernetes object for %s hook %s: %w", hook, h.Path, err) } // Record the time at which the hook was applied to the cluster @@ -77,12 +95,12 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, kube.ClientCreateOptionServerSideApply(serverSideApply, false)); err != nil { h.LastRun.CompletedAt = time.Now() h.LastRun.Phase = release.HookPhaseFailed - return fmt.Errorf("warning: Hook %s %s failed: %w", hook, h.Path, err) + return ShutdownNoOp, fmt.Errorf("warning: Hook %s %s failed: %w", hook, h.Path, err) } waiter, err := cfg.KubeClient.GetWaiter(waitStrategy) if err != nil { - return fmt.Errorf("unable to get waiter: %w", err) + return ShutdownNoOp, fmt.Errorf("unable to get waiter: %w", err) } // Watch hook resources until they have completed err = waiter.WatchUntilReady(resources, timeout) @@ -98,36 +116,39 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, } // If a hook is failed, check the annotation of the hook to determine whether the hook should be deleted // under failed condition. If so, then clear the corresponding resource object in the hook - if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, waitStrategy, timeout); errDeleting != nil { - // We log the error here as we want to propagate the hook failure upwards to the release object. - log.Printf("error deleting the hook resource on hook failure: %v", errDeleting) - } - - // If a hook is failed, check the annotation of the previous successful hooks to determine whether the hooks - // should be deleted under succeeded condition. - if err := cfg.deleteHooksByPolicy(executingHooks[0:i], release.HookSucceeded, waitStrategy, timeout); err != nil { + return func() error { + if errDeleting := cfg.deleteHookByPolicy(h, release.HookFailed, waitStrategy, timeout); errDeleting != nil { + // We log the error here as we want to propagate the hook failure upwards to the release object. + log.Printf("error deleting the hook resource on hook failure: %v", errDeleting) + } + + // If a hook is failed, check the annotation of the previous successful hooks to determine whether the hooks + // should be deleted under succeeded condition. + if err := cfg.deleteHooksByPolicy(executingHooks[0:i], release.HookSucceeded, waitStrategy, timeout); err != nil { + return err + } return err - } + }, err - return err } h.LastRun.Phase = release.HookPhaseSucceeded } - // If all hooks are successful, check the annotation of each hook to determine whether the hook should be deleted - // or output should be logged under succeeded condition. If so, then clear the corresponding resource object in each hook - for i := len(executingHooks) - 1; i >= 0; i-- { - h := executingHooks[i] - if err := cfg.outputLogsByPolicy(h, rl.Namespace, release.HookOutputOnSucceeded); err != nil { - // We log here as we still want to attempt hook resource deletion even if output logging fails. - log.Printf("error outputting logs for hook failure: %v", err) - } - if err := cfg.deleteHookByPolicy(h, release.HookSucceeded, waitStrategy, timeout); err != nil { - return err + return func() error { + // If all hooks are successful, check the annotation of each hook to determine whether the hook should be deleted + // or output should be logged under succeeded condition. If so, then clear the corresponding resource object in each hook + for i := len(executingHooks) - 1; i >= 0; i-- { + h := executingHooks[i] + if err := cfg.outputLogsByPolicy(h, rl.Namespace, release.HookOutputOnSucceeded); err != nil { + // We log here as we still want to attempt hook resource deletion even if output logging fails. + log.Printf("error outputting logs for hook failure: %v", err) + } + if err := cfg.deleteHookByPolicy(h, release.HookSucceeded, waitStrategy, timeout); err != nil { + return err + } } - } - - return nil + return nil + }, nil } // hookByWeight is a sorter for hooks diff --git a/pkg/action/release_testing.go b/pkg/action/release_testing.go index b649579f4..2aa063515 100644 --- a/pkg/action/release_testing.go +++ b/pkg/action/release_testing.go @@ -57,24 +57,24 @@ func NewReleaseTesting(cfg *Configuration) *ReleaseTesting { } // Run executes 'helm test' against the given release. -func (r *ReleaseTesting) Run(name string) (ri.Releaser, error) { +func (r *ReleaseTesting) Run(name string) (ri.Releaser, ExecuteShutdownHooks, error) { if err := r.cfg.KubeClient.IsReachable(); err != nil { - return nil, err + return nil, ShutdownNoOp, err } if err := chartutil.ValidateReleaseName(name); err != nil { - return nil, fmt.Errorf("releaseTest: Release name is invalid: %s", name) + return nil, ShutdownNoOp, fmt.Errorf("releaseTest: Release name is invalid: %s", name) } // finds the non-deleted release with the given name reli, err := r.cfg.Releases.Last(name) if err != nil { - return reli, err + return reli, ShutdownNoOp, err } rel, err := releaserToV1Release(reli) if err != nil { - return rel, err + return reli, ShutdownNoOp, err } skippedHooks := []*release.Hook{} @@ -102,14 +102,16 @@ func (r *ReleaseTesting) Run(name string) (ri.Releaser, error) { } serverSideApply := rel.ApplyMethod == string(release.ApplyMethodServerSideApply) - if err := r.cfg.execHook(rel, release.HookTest, kube.StatusWatcherStrategy, r.Timeout, serverSideApply); err != nil { + shutdown, err := r.cfg.execHookWithDelayedShutdown(rel, release.HookTest, kube.StatusWatcherStrategy, r.Timeout, serverSideApply) + + if err != nil { rel.Hooks = append(skippedHooks, rel.Hooks...) - r.cfg.Releases.Update(rel) - return rel, err + r.cfg.Releases.Update(reli) + return reli, shutdown, err } rel.Hooks = append(skippedHooks, rel.Hooks...) - return rel, r.cfg.Releases.Update(rel) + return reli, shutdown, r.cfg.Releases.Update(reli) } // GetPodLogs will write the logs for all test pods in the given release into diff --git a/pkg/cmd/release_testing.go b/pkg/cmd/release_testing.go index 88a6f351f..3d6921e71 100644 --- a/pkg/cmd/release_testing.go +++ b/pkg/cmd/release_testing.go @@ -65,7 +65,8 @@ func newReleaseTestCmd(cfg *action.Configuration, out io.Writer) *cobra.Command client.Filters[action.ExcludeNameFilter] = append(client.Filters[action.ExcludeNameFilter], notName.ReplaceAllLiteralString(f, "")) } } - reli, runErr := client.Run(args[0]) + reli, shutdown, runErr := client.Run(args[0]) + defer shutdown() // We only return an error if we weren't even able to get the // release, otherwise we keep going so we can print status and logs // if requested From 10714772bedf15c207bccf85be21f2e244d4a4ff Mon Sep 17 00:00:00 2001 From: Michelle Fernandez Bieber Date: Sun, 23 Jan 2022 00:17:09 +0100 Subject: [PATCH 2/8] updated comment and made defer of shutdown function return errors as before and not the possible shutdown error Signed-off-by: Michelle Fernandez Bieber --- pkg/action/hooks.go | 2 +- pkg/cmd/release_testing.go | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index c86964c65..3a0aabb61 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -49,7 +49,7 @@ func ShutdownNoOp() error { return nil } -// execHook executes all of the hooks for the given hook event and return a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs. +// execHook executes all of the hooks for the given hook event and returns a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs. func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (ExecuteShutdownHooks, error) { executingHooks := []*release.Hook{} diff --git a/pkg/cmd/release_testing.go b/pkg/cmd/release_testing.go index 3d6921e71..30de160ed 100644 --- a/pkg/cmd/release_testing.go +++ b/pkg/cmd/release_testing.go @@ -55,7 +55,7 @@ func newReleaseTestCmd(cfg *action.Configuration, out io.Writer) *cobra.Command } return compListReleases(toComplete, args, cfg) }, - RunE: func(_ *cobra.Command, args []string) error { + RunE: func(_ *cobra.Command, args []string) (returnError error) { client.Namespace = settings.Namespace() notName := regexp.MustCompile(`^!\s?name=`) for _, f := range filter { @@ -65,8 +65,17 @@ func newReleaseTestCmd(cfg *action.Configuration, out io.Writer) *cobra.Command client.Filters[action.ExcludeNameFilter] = append(client.Filters[action.ExcludeNameFilter], notName.ReplaceAllLiteralString(f, "")) } } + reli, shutdown, runErr := client.Run(args[0]) - defer shutdown() + defer func() { + if shutdownErr := shutdown(); shutdownErr != nil { + if returnError == nil { + returnError = shutdownErr + } + } + }() + + // We only return an error if we weren't even able to get the // release, otherwise we keep going so we can print status and logs // if requested From 7a61ebf01370d67684681c0d210ddec90fda3503 Mon Sep 17 00:00:00 2001 From: Michelle Fernandez Bieber Date: Sun, 23 Jan 2022 00:25:32 +0100 Subject: [PATCH 3/8] cleaned up empty line Signed-off-by: Michelle Fernandez Bieber --- pkg/action/hooks.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 3a0aabb61..bd8e96e74 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -129,7 +129,6 @@ func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook } return err }, err - } h.LastRun.Phase = release.HookPhaseSucceeded } From d9301441f47bb8cf4c84ffcbed22f39ba70b1588 Mon Sep 17 00:00:00 2001 From: Michelle Fernandez Bieber Date: Fri, 22 Apr 2022 22:55:35 +0200 Subject: [PATCH 4/8] added check for nil shutdown Signed-off-by: Michelle Fernandez Bieber --- pkg/action/hooks.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index bd8e96e74..adf7478f6 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -34,6 +34,9 @@ import ( // execHook executes all of the hooks for the given hook event. func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) error { shutdown, err := cfg.execHookWithDelayedShutdown(rl, hook, waitStrategy, timeout, serverSideApply) + if shutdown == nil { + return nil + } if err != nil { if err := shutdown(); err != nil { return err From 6bb5bcc212fd014ad521eab8b1ffd94f3f4f0f71 Mon Sep 17 00:00:00 2001 From: Michelle Fernandez Bieber <37021266+mfbieber@users.noreply.github.com> Date: Sun, 23 Jul 2023 08:17:57 +0200 Subject: [PATCH 5/8] Update pkg/action/hooks.go Co-authored-by: Marco Lecheler Signed-off-by: Michelle Fernandez Bieber <37021266+mfbieber@users.noreply.github.com> --- pkg/action/hooks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index adf7478f6..c336e10fe 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -52,7 +52,7 @@ func ShutdownNoOp() error { return nil } -// execHook executes all of the hooks for the given hook event and returns a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs. +// execHookWithDelayedShutdown executes all of the hooks for the given hook event and returns a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs. func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (ExecuteShutdownHooks, error) { executingHooks := []*release.Hook{} From 9f1c8a26f00ebbe2942064a06b49c275162d10ef Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Wed, 26 Nov 2025 17:24:06 +0100 Subject: [PATCH 6/8] Fix linting issue Signed-off-by: Benoit Tigeot --- pkg/cmd/release_testing.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/release_testing.go b/pkg/cmd/release_testing.go index 30de160ed..5a6159e7d 100644 --- a/pkg/cmd/release_testing.go +++ b/pkg/cmd/release_testing.go @@ -75,7 +75,6 @@ func newReleaseTestCmd(cfg *action.Configuration, out io.Writer) *cobra.Command } }() - // We only return an error if we weren't even able to get the // release, otherwise we keep going so we can print status and logs // if requested From 70fc5f97e2b42a2cf87ff8756d6baf4ab2d5a920 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Wed, 26 Nov 2025 23:29:57 +0100 Subject: [PATCH 7/8] Code review Signed-off-by: Benoit Tigeot --- pkg/action/hooks.go | 16 ++++++++-------- pkg/action/release_testing.go | 10 +++++----- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index c336e10fe..2ea851a3e 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -35,7 +35,7 @@ import ( func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) error { shutdown, err := cfg.execHookWithDelayedShutdown(rl, hook, waitStrategy, timeout, serverSideApply) if shutdown == nil { - return nil + return err } if err != nil { if err := shutdown(); err != nil { @@ -46,14 +46,14 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, return shutdown() } -type ExecuteShutdownHooks = func() error +type executeShutdownFunc = func() error -func ShutdownNoOp() error { +func shutdownNoOp() error { return nil } // execHookWithDelayedShutdown executes all of the hooks for the given hook event and returns a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs. -func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (ExecuteShutdownHooks, error) { +func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (executeShutdownFunc, error) { executingHooks := []*release.Hook{} for _, h := range rl.Hooks { @@ -72,12 +72,12 @@ func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook cfg.hookSetDeletePolicy(h) if err := cfg.deleteHookByPolicy(h, release.HookBeforeHookCreation, waitStrategy, timeout); err != nil { - return ShutdownNoOp, err + return shutdownNoOp, err } resources, err := cfg.KubeClient.Build(bytes.NewBufferString(h.Manifest), true) if err != nil { - return ShutdownNoOp, fmt.Errorf("unable to build kubernetes object for %s hook %s: %w", hook, h.Path, err) + return shutdownNoOp, fmt.Errorf("unable to build kubernetes object for %s hook %s: %w", hook, h.Path, err) } // Record the time at which the hook was applied to the cluster @@ -98,12 +98,12 @@ func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook kube.ClientCreateOptionServerSideApply(serverSideApply, false)); err != nil { h.LastRun.CompletedAt = time.Now() h.LastRun.Phase = release.HookPhaseFailed - return ShutdownNoOp, fmt.Errorf("warning: Hook %s %s failed: %w", hook, h.Path, err) + return shutdownNoOp, fmt.Errorf("warning: Hook %s %s failed: %w", hook, h.Path, err) } waiter, err := cfg.KubeClient.GetWaiter(waitStrategy) if err != nil { - return ShutdownNoOp, fmt.Errorf("unable to get waiter: %w", err) + return shutdownNoOp, fmt.Errorf("unable to get waiter: %w", err) } // Watch hook resources until they have completed err = waiter.WatchUntilReady(resources, timeout) diff --git a/pkg/action/release_testing.go b/pkg/action/release_testing.go index 2aa063515..673a678f5 100644 --- a/pkg/action/release_testing.go +++ b/pkg/action/release_testing.go @@ -57,24 +57,24 @@ func NewReleaseTesting(cfg *Configuration) *ReleaseTesting { } // Run executes 'helm test' against the given release. -func (r *ReleaseTesting) Run(name string) (ri.Releaser, ExecuteShutdownHooks, error) { +func (r *ReleaseTesting) Run(name string) (ri.Releaser, executeShutdownFunc, error) { if err := r.cfg.KubeClient.IsReachable(); err != nil { - return nil, ShutdownNoOp, err + return nil, shutdownNoOp, err } if err := chartutil.ValidateReleaseName(name); err != nil { - return nil, ShutdownNoOp, fmt.Errorf("releaseTest: Release name is invalid: %s", name) + return nil, shutdownNoOp, fmt.Errorf("releaseTest: Release name is invalid: %s", name) } // finds the non-deleted release with the given name reli, err := r.cfg.Releases.Last(name) if err != nil { - return reli, ShutdownNoOp, err + return reli, shutdownNoOp, err } rel, err := releaserToV1Release(reli) if err != nil { - return reli, ShutdownNoOp, err + return reli, shutdownNoOp, err } skippedHooks := []*release.Hook{} From 45c5f3aaca1a37d8388ce7b79efe2dfaf84dcdcc Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Thu, 27 Nov 2025 08:48:31 +0100 Subject: [PATCH 8/8] Deal with golint warning with private executeShutdownFunc ``` Error: pkg/action/release_testing.go:60:57: unexported-return: exported method Run returns unexported type action.executeShutdownFunc, which can be annoying to use (revive) func (r *ReleaseTesting) Run(name string) (ri.Releaser, executeShutdownFunc, error) { ``` Signed-off-by: Benoit Tigeot --- pkg/action/hooks.go | 4 ++-- pkg/action/release_testing.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 2ea851a3e..49849a27d 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -46,14 +46,14 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, return shutdown() } -type executeShutdownFunc = func() error +type ExecuteShutdownFunc = func() error func shutdownNoOp() error { return nil } // execHookWithDelayedShutdown executes all of the hooks for the given hook event and returns a shutdownHook function to trigger deletions after doing other things like e.g. retrieving logs. -func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (executeShutdownFunc, error) { +func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook release.HookEvent, waitStrategy kube.WaitStrategy, timeout time.Duration, serverSideApply bool) (ExecuteShutdownFunc, error) { executingHooks := []*release.Hook{} for _, h := range rl.Hooks { diff --git a/pkg/action/release_testing.go b/pkg/action/release_testing.go index 673a678f5..992cdd701 100644 --- a/pkg/action/release_testing.go +++ b/pkg/action/release_testing.go @@ -57,7 +57,7 @@ func NewReleaseTesting(cfg *Configuration) *ReleaseTesting { } // Run executes 'helm test' against the given release. -func (r *ReleaseTesting) Run(name string) (ri.Releaser, executeShutdownFunc, error) { +func (r *ReleaseTesting) Run(name string) (ri.Releaser, ExecuteShutdownFunc, error) { if err := r.cfg.KubeClient.IsReachable(); err != nil { return nil, shutdownNoOp, err }