feat: PreDelete hooks support (Issue #13975) #22288
Conversation
Signed-off-by: Dan Garfield <[email protected]> Signed-off-by: Pedro Ribeiro <[email protected]>
…#22211) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Pedro Ribeiro <[email protected]>
…rgoproj#22104) (argoproj#22208) Signed-off-by: Andrii Korotkov <[email protected]> Signed-off-by: Pedro Ribeiro <[email protected]>
argoproj#22117) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ishita Sequeira <[email protected]> Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
…delete-hooks # Conflicts: # go.mod # go.sum # ui-test/package.json
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22288 +/- ##
==========================================
+ Coverage 56.06% 56.09% +0.03%
==========================================
Files 343 343
Lines 57536 57615 +79
==========================================
+ Hits 32257 32319 +62
- Misses 22634 22640 +6
- Partials 2645 2656 +11 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
|
Yesterday I was implementing a workaround because of this missing feature. Now I think holy sh*t cow there it is. |
# Conflicts: # go.mod
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
|
@andrii-korotkov-verkada Thanks for the Review! I rebased the code correctly now and implemented some changes according to your review, please check. |
Signed-off-by: Pedro Ribeiro <[email protected]>
…d into pre-delete-hooks
Signed-off-by: Pedro Ribeiro <[email protected]>
docs/user-guide/resource_hooks.md
Outdated
| @@ -9,6 +9,7 @@ and after a Sync operation. Hooks can also be run if a Sync operation fails at a | |||
| Kubernetes rolling update strategy. | |||
| * Using a `PostSync` hook to run integration and health checks after a deployment. | |||
| * Using a `SyncFail` hook to run clean-up or finalizer logic if a Sync operation fails. | |||
| * Using a `PreDelete` hook to run tasks before an Application's resources are deleted. This can be useful for graceful shutdowns, backup operations, or other pre-deletion tasks. Similar to `PostDelete` hooks, `PreDelete` hooks are executed when the application is being deleted, and are managed separately from the application lifecycle. | |||
There was a problem hiding this comment.
What happens if I remove a resource from git and sync with prune option enabled? Will the PreDelete hook be executed? It is better to clarify in the docs in which exact conditions the hook gets triggered.
There was a problem hiding this comment.
Agreed. I updated the docs dcc55ec (#22288) to clarify the conditions where the hook gets triggered
There was a problem hiding this comment.
We should have unit-tests validating this logic
There was a problem hiding this comment.
Added the hook_test.go tests here: 33c2007 (#22288)
controller/appcontroller.go
Outdated
| if app.HasPreDeleteFinalizer() { | ||
| objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please return the context alongside with errors.
Example:
return fmt.Errorf("error getting permitted app live objects: %w", err)There was a problem hiding this comment.
Updated the code to return the context now with the error: 6a864c8 (#22288)
controller/appcontroller.go
Outdated
|
|
||
| done, err := ctrl.executePreDeleteHooks(app, proj, objsMap, config, logCtx) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please return the context alongside with errors.
Example:
return fmt.Errorf("error getting permitted app live objects: %w", err)There was a problem hiding this comment.
Updated the code to return the context now with the error: 6a864c8 (#22288)
controller/appcontroller.go
Outdated
| // PreDelete hooks are done - remove the finalizer so we can continue with deletion | ||
| app.UnSetPreDeleteFinalizer() | ||
| if err := ctrl.updateFinalizers(app); err != nil { | ||
| return err |
There was a problem hiding this comment.
Please return the context alongside with errors.
Example:
return fmt.Errorf("error getting permitted app live objects: %w", err)There was a problem hiding this comment.
Updated the code to return the context now with the error: 6a864c8 (#22288)
controller/appcontroller.go
Outdated
| if app.HasPreDeleteFinalizer("cleanup") { | ||
| objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please return the context alongside with errors.
Example:
return fmt.Errorf("error getting permitted app live objects: %w", err)There was a problem hiding this comment.
Updated the code to return the context now with the error: 6a864c8 (#22288)
controller/appcontroller.go
Outdated
|
|
||
| done, err := ctrl.cleanupPreDeleteHooks(objsMap, config, logCtx) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Please return the context alongside with errors.
Example:
return fmt.Errorf("error getting permitted app live objects: %w", err)There was a problem hiding this comment.
Updated the code to return the context now with the error: 6a864c8 (#22288)
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
Signed-off-by: Pedro Ribeiro <[email protected]>
|
Thanks for the review @leoluz ! I did some changes regarding your comments, PTAL |
Signed-off-by: Pedro Ribeiro <[email protected]>
Closes #13975
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:
Checklist:
ArgoCD_PreDelete_Hook.mov