feat: PreDelete hooks support (Issue #13975) #22288

pedro-cs-ribeiro · 2025-03-10T17:15:58Z

This PR introduces PreDelete hooks for Argo CD. PreDelete hooks allow users to execute tasks before an application's resources are deleted, providing a way to perform graceful shutdowns, backup operations, or other cleanup tasks that should happen before resource deletion.

Implementation details:

The implementation follows the same pattern established for PostDelete hooks:

Adds PreDeleteFinalizerName constant
Implements Pre/UnSet/HasPreDeleteFinalizer methods
Adds hook detection logic via isPreDeleteHook()
Implements hook execution methods via executePreDeleteHooks() and cleanupPreDeleteHooks()
Updates application controller to respect the proper deletion order

Checklist:

ArgoCD_PreDelete_Hook.mov

Signed-off-by: Dan Garfield <dan@codefresh.io> Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

…#22211) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

…rgoproj#22104) (argoproj#22208) Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

argoproj#22117) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

…delete-hooks # Conflicts: # go.mod # go.sum # ui-test/package.json

bunnyshell · 2025-03-10T17:16:03Z

✅ Preview Environment deployed on Bunnyshell

Component	Endpoints
argocd	https://argocd-4blfmh.bunnyenv.com/
argocd-ttyd	https://argocd-web-cli-4blfmh.bunnyenv.com/

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🔴 /bns:stop to stop the environment
🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to remove the environment

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

codecov · 2025-03-10T19:44:02Z

Codecov Report

Attention: Patch coverage is 53.64583% with 89 lines in your changes missing coverage. Please review.

Project coverage is 55.87%. Comparing base (111cf2c) to head (0071ca3).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
controller/hook.go	52.34%	51 Missing and 10 partials ⚠️
controller/appcontroller.go	46.15%	19 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #22288      +/-   ##
==========================================
- Coverage   55.91%   55.87%   -0.04%     
==========================================
  Files         343      343              
  Lines       57305    57487     +182     
==========================================
+ Hits        32042    32121      +79     
- Misses      22623    22705      +82     
- Partials     2640     2661      +21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

controller/hook.go

andrii-korotkov-verkada · 2025-03-10T20:07:23Z

controller/hook.go

+			hookHealth = &health.HealthStatus{
+				Status: health.HealthStatusHealthy,
+			}
+		}


Is this necessary? I don't know if assuming healthy for when there's no health would not have any undesired side effects.

For pre-delete hooks, defaulting to "Healthy" when no health check is defined prevents the process from getting stuck waiting for an undefined condition.
This approach is consistent with how we handle other hook types on the code.

andrii-korotkov-verkada · 2025-03-10T20:07:47Z

server/application/deepinformer_test.go

@@ -86,7 +85,7 @@ func Test_deepCopyAppProjectClient_List(t *testing.T) {
 		{name: "Error listing app project", fields: fields{
 			AppProjectInterface: func() clientset.AppProjectInterface {
 				appProject := mocks.AppProjectInterface{}
-				appProject.On("List", context.Background(), metav1.ListOptions{}).Return(nil, errors.New("error"))
+				appProject.On("List", t.Context(), metav1.ListOptions{}).Return(nil, errors.New("error"))


Need to rebase, this was fixed in another PR

Rebased to latest upstream master

Looks like rebase wasn't fully done or wasn't to the latest master, since these changes are still here.

) Signed-off-by: Atif Ali <atali@redhat.com> Co-authored-by: Dan Garfield <dan@codefresh.io>

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

andrii-korotkov-verkada · 2025-03-10T20:52:15Z

controller/appcontroller.go

 	destCluster, err := argo.GetDestinationCluster(context.Background(), app.Spec.Destination, ctrl.db)
 	if err != nil {
-		logCtx.Warnf("Unable to get destination cluster: %v", err)
+		logCtx.Warnf("Unable to locate cluster URL for Application being deleted: %v", err)


Is this accurate? What if we can locate a URL, but there's some other error getting the cluster?

andrii-korotkov-verkada · 2025-03-10T20:52:31Z

controller/appcontroller.go

 		app.UnSetCascadedDeletion()
 		app.UnSetPostDeleteFinalizer()
+		if app.HasPreDeleteFinalizer() {
+			app.UnSetPreDeleteFinalizer()
+		}


Why doesn't app.UnSetPostDeleteFinalizer() require such a check?

andrii-korotkov-verkada · 2025-03-10T20:54:33Z

controller/hook.go

 	postDeleteHooks = map[string]string{
 		"argocd.argoproj.io/hook": postDeleteHook,
 		"helm.sh/hook":            "post-delete",
 	}
 )

 func isHook(obj *unstructured.Unstructured) bool {
-	return hook.IsHook(obj) || isPostDeleteHook(obj)
+	return hook.IsHook(obj) || isPostDeleteHook(obj) || isPreDeleteHook(obj)


Why are pre- and post- delete hooks are handled separately?

andrii-korotkov-verkada · 2025-03-10T20:55:09Z

controller/hook.go

+	return true, nil
+}
+
+func (ctrl *ApplicationController) executePostDeleteHooks(app *appv1.Application, proj *appv1.AppProject, liveObjs map[kube.ResourceKey]*unstructured.Unstructured, config *rest.Config, logCtx *log.Entry) (bool, error) {


Can we unify some code with post-detele hooks?

andrii-korotkov-verkada · 2025-03-10T20:55:48Z

server/application/deepinformer_test.go

@@ -86,7 +85,7 @@ func Test_deepCopyAppProjectClient_List(t *testing.T) {
 		{name: "Error listing app project", fields: fields{
 			AppProjectInterface: func() clientset.AppProjectInterface {
 				appProject := mocks.AppProjectInterface{}
-				appProject.On("List", context.Background(), metav1.ListOptions{}).Return(nil, errors.New("error"))
+				appProject.On("List", t.Context(), metav1.ListOptions{}).Return(nil, errors.New("error"))


Looks like rebase wasn't fully done or wasn't to the latest master, since these changes are still here.

andrii-korotkov-verkada · 2025-03-10T20:56:18Z

ui/src/app/settings/components/repos-list/repos-list.tsx

-                        (httpsValues.type != 'git' && 'Bearer token is only supported for Git BitBucket Data Center repositories'),
-                    tlsClientCertKey: !httpsValues.tlsClientCertKey && httpsValues.tlsClientCertData && 'TLS client cert key is required if TLS client cert is given.'
+                        (validURLValues.password && validURLValues.bearerToken && 'Either the password or the bearer token must be set, but not both.') ||
+                        (validURLValues.type != 'git' && 'Bearer token is only supported for Git BitBucket Data Center repositories.')


Why are these changes needed?

andrii-korotkov-verkada · 2025-03-10T20:56:42Z

ui/src/app/settings/components/repos-list/repos-list.tsx

-    private isHTTPSUrl(url: string) {
-        if (url.match(/^https:\/\/.*$/gi)) {
+    // Whether url is a http or https url
+    private isHTTPOrHTTPSUrl(url: string) {


Can we separate these changes into a different PR, please?

todaywasawesome and others added 8 commits March 10, 2025 17:03

docs: Fix typos and grammar in tls.md (argoproj#22229)

e76e6d4

Signed-off-by: Dan Garfield <dan@codefresh.io> Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

chore(deps): bump golang.org/x/oauth2 from 0.27.0 to 0.28.0 (argoproj…

f6733d8

…#22211) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

chore: Update some dependencies and add some comments about old libs (a…

60dbdc2

…rgoproj#22104) (argoproj#22208) Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com> Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

feat: PreDelete hooks support

230b328

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

feat: Add PreDelete hook tests

d17e0f6

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

feat: Update docs with PreDelete hook references

b676d24

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

Merge remote-tracking branch 'refs/remotes/upstream/master' into pre-…

72aaa09

…delete-hooks # Conflicts: # go.mod # go.sum # ui-test/package.json

pedro-cs-ribeiro requested review from a team as code owners March 10, 2025 17:15

pedro-ribeiro-rci and others added 4 commits March 10, 2025 17:28

fix: fix e2e test import

501806d

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

fix: fix lint

2455a76

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

fix: fix lint

d7be2a9

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

chore(lint): fix deep copy informers lint (argoproj#22290)

42219fd

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

andrii-korotkov-verkada reviewed Mar 10, 2025

View reviewed changes

aali309 and others added 8 commits March 10, 2025 20:16

fix: Unable to edit http repo credentials from ArgoCD UI (argoproj#22065

0ed7c56

) Signed-off-by: Atif Ali <atali@redhat.com> Co-authored-by: Dan Garfield <dan@codefresh.io>

feat: PreDelete hooks support

1a5abe1

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

feat: Add PreDelete hook tests

79dc586

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

feat: Update docs with PreDelete hook references

42effb4

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

fix: fix e2e test import

9399fdf

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

fix: fix lint

ef48c4f

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

fix: fix pr comments

4e60963

Signed-off-by: Pedro Ribeiro <pedro.ribeiro@cross-join.com>

merge

0071ca3

andrii-korotkov-verkada reviewed Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: PreDelete hooks support (Issue #13975) #22288

feat: PreDelete hooks support (Issue #13975) #22288

pedro-cs-ribeiro commented Mar 10, 2025

bunnyshell bot commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

andrii-korotkov-verkada Mar 10, 2025

pedro-cs-ribeiro Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

pedro-cs-ribeiro Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

andrii-korotkov-verkada Mar 10, 2025

feat: PreDelete hooks support (Issue #13975) #22288

Are you sure you want to change the base?

feat: PreDelete hooks support (Issue #13975) #22288

Conversation

pedro-cs-ribeiro commented Mar 10, 2025

bunnyshell bot commented Mar 10, 2025 • edited Loading

✅ Preview Environment deployed on Bunnyshell

codecov bot commented Mar 10, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bunnyshell bot commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading