feat: add Prune and Delete as application level sync option#23370
feat: add Prune and Delete as application level sync option#23370agaudreault merged 5 commits intoargoproj:masterfrom
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
f5381c1 to
b2ca286
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23370 +/- ##
==========================================
+ Coverage 62.98% 63.03% +0.04%
==========================================
Files 414 414
Lines 56153 56198 +45
==========================================
+ Hits 35370 35425 +55
+ Misses 17416 17409 -7
+ Partials 3367 3364 -3 ☔ View full report in Codecov by Sentry. |
b2ca286 to
5d5a4c3
Compare
pjiang-dev
left a comment
There was a problem hiding this comment.
Could you create an issue describing what the issue/enhancement is and why this change is needed and link it to this PR and your gitops-engine PR?
Sure! I just did that here: #23380 |
|
The deletion part LGTM. Maybe helpful to add a screenshot or video of the sync option working. Thanks |
docs/user-guide/sync-options.md
Outdated
| argocd.argoproj.io/sync-options: Prune=false | ||
| ``` | ||
|
|
||
| It also can be enabled at the application level like in the example below: |
There was a problem hiding this comment.
Also just one question, if an application's resource has prune=true enabled, but the application has prune=false, which one takes priority? I am assuming resource level should take priority ? Same with the other options.
If not documented somewhere already i think its worth mentioning in the docs
There was a problem hiding this comment.
The Prune/Delete=true are not an actual thing, it's just something added in the UI that can act as a default option. Prune=helloworld would be similarly handled technically :p.
For the rest of the options there is not really a priority thing currently, both behavior will apply. For instance if you have Delete=confirm and Delete=false on some resources it will ask you for confirmation and do not delete the resource marked as delete=false (and similarly with prune). I will add clarification for that in docs, thanks!
There was a problem hiding this comment.
Done! Let me know if the new doc is more clear!
5d5a4c3 to
5ab9a2b
Compare
Sure, will try to provide a video later today! |
|
(rebasing for the go.mod conflict below and bumping the gitops-engine fork, no other code change) |
98975a0 to
12498ec
Compare
|
@pjiang-dev here is the video (I had to fix a dumb typo https://github.com/argoproj/gitops-engine/compare/13f9343d366f59f29905dda9c8ef4e6257a67c67..03789b820ebec73de41af43a35493983c1832e01 in the gitops-engine so thanks for making me double check 🙈) recording.mp4 |
|
FYI here is the yaml that I am using in the video above: |
docs/user-guide/sync-options.md
Outdated
| It also can be enabled at the application level like in the example below: | ||
|
|
||
| ```yaml | ||
| apiVersion: argoproj.io/v1alpha1 | ||
| kind: Application | ||
| spec: | ||
| syncPolicy: | ||
| syncOptions: | ||
| - Prune=confirm | ||
| ``` | ||
|
|
||
| Note that setting a Prune sync option on the application level will not override | ||
| the same sync option set on a specific resource, both will still be applied. | ||
|
|
There was a problem hiding this comment.
Instead of repeating the doc, specify that the same value can be use either through the annotation or the sync option.
There was a problem hiding this comment.
I don't mind doing that but IIUC adding both like that is consistent with the current doc, for instance ServerSideApply: https://argo-cd.readthedocs.io/en/stable/user-guide/sync-options/#server-side-apply
ui/src/app/applications/components/application-sync-options/application-sync-options.tsx
Outdated
Show resolved
Hide resolved
12498ec to
38e4817
Compare
|
Hi @agaudreault, kind reminder about this PR, would love to have your updated input on the matter whenever you have time do so 🙏 |
|
Hi @agaudreault small reminder about this PR in case it drop of your radar 🙏 |
ae73ef9 to
48858af
Compare
5fc1319 to
71ffdba
Compare
|
@agaudreault FYI I just rebased this PR on master now that this #25915 has been merged (thanks again!), and fixed some issues that popped on the new CI and this should be ready again for review! |
|
Hi @agaudreault kind reminder abut this, it would be super awesome if this could make it in time for the 3.4 release 🙏 |
|
I just rebased to make the CI the "Ensure Go modules synchronicity" test pass (I am not sure why it fails though since I don't update the go.mod/any dependency here, perhaps it's when updating the branch via merge 🤷♂️) EDIT: Nevermind it was actually related to this PR with |
c5ba5a1 to
dd9650c
Compare
agaudreault
left a comment
There was a problem hiding this comment.
Overall, the feature LGTM. Few refactors required here and there.
ui/src/app/applications/components/application-sync-options/application-sync-options.tsx
Outdated
Show resolved
Hide resolved
This will facilitate adding logic on sync options that can be defined both at the app and resource level and the resource level should override the app level option. Also fix GetAnnotationCSVs to preserve the order returned (as map in golang doesn't preserve the insertion order) to make dedup works in GetOptionValue. Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>
dd9650c to
ac9fd6e
Compare
Add support for Delete and Prune sync options on the application level. When at the application level those options are only changing the "default behavior" for resources deployed by this application. The behavior is still to process this at the resource level and the resource level always override the app level Delete option. Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>
019fa7d to
874246b
Compare
test/e2e/app_management_test.go
Outdated
| t.Context(), &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-configmap", | ||
| Labels: map[string]string{ |
There was a problem hiding this comment.
You need to set the tracking annotation correctly here instead of an app instance label.
There was a problem hiding this comment.
Ohh thanks a lot! The test are green now 🎉
agaudreault
left a comment
There was a problem hiding this comment.
LGTM. Ready to merge when test are fixed.
Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>
Also reorder the PruneLast option so that all prune options are grouped Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>
874246b to
a3e5ec9
Compare
Checklist:
My goal was mainly to add Prune/Delete=confirm as a sync option as the deletion confirmation acts on the whole Application already and it's way more convenient in our env to add it on the application level than as annotation on the resources actually deployed.
For consistency I also added Prune/Delete=false support there (separate commit).
And the UI option in when editing the sync policy for the Prune and Delete sync options
Closes #23380