From 2e728437ba723fddc0117d6dbc4208f4997013bd Mon Sep 17 00:00:00 2001
From: Laszlo Uveges <laszlo@giantswarm.io>
Date: Mon, 12 Sep 2022 16:52:14 +0200
Subject: [PATCH] Delete previously successful hooks when a later hook fails

Signed-off-by: Laszlo Uveges <laszlo@giantswarm.io>
---
 pkg/action/hooks.go      | 26 +++++++++++++++++++++-----
 pkg/action/hooks_test.go |  8 ++++++++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go
index 40c1ffdb6..4fbe28bbe 100644
--- a/pkg/action/hooks.go
+++ b/pkg/action/hooks.go
@@ -41,7 +41,7 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent,
 	// hooke are pre-ordered by kind, so keep order stable
 	sort.Stable(hookByWeight(executingHooks))
 
-	for _, h := range executingHooks {
+	for i, h := range executingHooks {
 		// Set default delete policy to before-hook-creation
 		if h.DeletePolicies == nil || len(h.DeletePolicies) == 0 {
 			// TODO(jlegrone): Only apply before-hook-creation delete policy to run to completion
@@ -91,6 +91,13 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent,
 			if err := cfg.deleteHookByPolicy(h, release.HookFailed); err != nil {
 				return err
 			}
+
+			// If a hook is failed, check the annotation of the previous successful hooks to determine whether the hook
+			// should be deleted under succeeded condition.
+			if err := cfg.deleteHooksByPolicy(executingHooks[0:i], release.HookSucceeded); err != nil {
+				return err
+			}
+
 			return err
 		}
 		h.LastRun.Phase = release.HookPhaseSucceeded
@@ -98,10 +105,8 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent,
 
 	// If all hooks are successful, check the annotation of each hook to determine whether the hook should be deleted
 	// under succeeded condition. If so, then clear the corresponding resource object in each hook
-	for _, h := range executingHooks {
-		if err := cfg.deleteHookByPolicy(h, release.HookSucceeded); err != nil {
-			return err
-		}
+	if err := cfg.deleteHooksByPolicy(executingHooks, release.HookSucceeded); err != nil {
+		return err
 	}
 
 	return nil
@@ -149,3 +154,14 @@ func hookHasDeletePolicy(h *release.Hook, policy release.HookDeletePolicy) bool
 	}
 	return false
 }
+
+// deleteHooksByPolicy deletes all hooks if the hook policy instructs it to
+func (cfg *Configuration) deleteHooksByPolicy(hooks []*release.Hook, policy release.HookDeletePolicy) error {
+	for _, h := range hooks {
+		if err := cfg.deleteHookByPolicy(h, policy); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
diff --git a/pkg/action/hooks_test.go b/pkg/action/hooks_test.go
index 0a3df94ef..bb9f39c1e 100644
--- a/pkg/action/hooks_test.go
+++ b/pkg/action/hooks_test.go
@@ -137,17 +137,25 @@ data:
 				Namespace: "test",
 			}, []resource.Info{
 				{
+					// This should be in the record for `before-hook-creation`
 					Name:      "build-config-1",
 					Namespace: "test",
 				},
 				{
+					// This should be in the record for `before-hook-creation`
 					Name:      "build-config-2",
 					Namespace: "test",
 				},
 				{
+					// This should be in the record for cleaning up (the failure first)
 					Name:      "build-config-2",
 					Namespace: "test",
 				},
+				{
+					// This should be in the record for cleaning up (then the previously successful)
+					Name:      "build-config-1",
+					Namespace: "test",
+				},
 			}, true,
 		},
 	}