Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(appset): ignoreApplicationDifferences not working #15965

Merged

Conversation

crenshaw-dev
Copy link
Member

@crenshaw-dev crenshaw-dev commented Oct 13, 2023

The original implementation failed to normalize both the live and the generated state, so the applied patch could contain fields which should have been ignored.

I did some refactoring here, but I think it was all necessary in order to properly normalize both manifests before calculating the patch.

🤖 Generated by Copilot at 7db7cc4

This pull request refactors and enhances the ApplicationSet controller and utils package to support ignoring differences between live and desired state of Application resources, and to log the patches that are applied. It also updates the documentation and adds unit tests for the new features.

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

codecov bot commented Oct 14, 2023

Codecov Report

Attention: 65 lines in your changes are missing coverage. Please review.

Comparison is base (d429013) 49.55% compared to head (d6034a6) 49.55%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #15965   +/-   ##
=======================================
  Coverage   49.55%   49.55%           
=======================================
  Files         269      269           
  Lines       46852    46891   +39     
=======================================
+ Hits        23216    23236   +20     
- Misses      21357    21374   +17     
- Partials     2279     2281    +2     
Files Coverage Δ
pkg/apis/application/v1alpha1/types.go 58.58% <100.00%> (ø)
util/argo/argo.go 66.62% <100.00%> (+0.12%) ⬆️
...cationset/controllers/applicationset_controller.go 60.78% <33.33%> (+1.24%) ⬆️
applicationset/utils/createOrUpdate.go 20.42% <32.22%> (+20.42%) ⬆️

... and 1 file with indirect coverage changes

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

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

Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev crenshaw-dev added this to the v2.9 milestone Oct 23, 2023
@crenshaw-dev crenshaw-dev marked this pull request as ready for review October 23, 2023 15:38
@crenshaw-dev crenshaw-dev requested review from a team as code owners October 23, 2023 15:38
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@mzupan
Copy link

mzupan commented Oct 29, 2023

tested this from a slack thread and its now working as expected for switching branches

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @crenshaw-dev. Tested the feature locally and overall code changes look good!

@ishitasequeira ishitasequeira enabled auto-merge (squash) October 30, 2023 03:13
ishitasequeira and others added 2 commits October 29, 2023 23:13
Signed-off-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@ishitasequeira ishitasequeira merged commit 28edaf5 into argoproj:master Oct 30, 2023
25 checks passed
@zhammer
Copy link

zhammer commented Nov 7, 2023

do we have an ETA on when this fix will be released? thank you for the quick turnaround

@mzupan
Copy link

mzupan commented Nov 7, 2023

@zhammer 2.9 was released yesterday

@zhammer
Copy link

zhammer commented Nov 7, 2023

@mzupan might be missing something simple, but looking at the commits for the v2.9.0 tag, this fix is missing v2.9.0...master

@mzupan
Copy link

mzupan commented Nov 7, 2023

@zhammer it's there/. you just need to click the load more commits button a few times..

@zhammer
Copy link

zhammer commented Nov 7, 2023

@mzupan ah, my fault, thank you.

@crenshaw-dev crenshaw-dev deleted the fix-appset-ignore-differences branch November 8, 2023 15:19
@math3vz
Copy link

math3vz commented Nov 9, 2023

@mzupan @crenshaw-dev hey guys, it looks like this feature is in the master branch but not in the 2.9.0 tag, am I missing something here?

2.9.0
https://github.com/argoproj/argo-cd/blob/v2.9.0/applicationset/controllers/applicationset_controller.go#L629 -> there's no appLog as second param

master
https://github.com/argoproj/argo-cd/blob/master/applicationset/controllers/applicationset_controller.go#L628 -> appLog is here

EDIT: I've been trying to use this feature using quay.io/argoproj/argocd:v2.9.0 and it didn't work, after changing the image to quay.io/argoproj/argocd:latest it did work

@ishitasequeira
Copy link
Member

/cherry-pick release-2.9

Copy link

Cherry-pick failed with Merge error 28edaf58b03d5b595d56084503661f69a2b5ad1e into temp-cherry-pick-64829f-release-2.9

@ishitasequeira
Copy link
Member

I don't see it in v2.9.0 either. Thanks for pointing that out. Let me cherry-pick for 2.9.1

@zhammer
Copy link

zhammer commented Nov 9, 2023

ah thank you!

@argoproj argoproj deleted a comment from gcp-cherry-pick-bot bot Nov 9, 2023
@ishitasequeira
Copy link
Member

/cherry-pick release-2.9

Copy link

Cherry-pick failed with Merge error 28edaf58b03d5b595d56084503661f69a2b5ad1e into temp-cherry-pick-64829f-release-2.9

@ishitasequeira
Copy link
Member

As cherry-pick bot is failing, created PR #16299 for cherry-picking the changes to 2.9.1

jmilic1 pushed a commit to jmilic1/argo-cd that referenced this pull request Nov 13, 2023
* fix(appset): ignoreApplicationDifferences not working

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

* tests, docs

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

* link to enhancement request

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

* handle error

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

* Update docs/operator-manual/applicationset/Controlling-Resource-Modification.md

Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix bug, fix docs

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

* fix docs

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

* normalize empty syncPolicy field

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

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Signed-off-by: jmilic1 <70441727+jmilic1@users.noreply.github.com>
crenshaw-dev added a commit that referenced this pull request Nov 13, 2023
* fix(appset): ignoreApplicationDifferences not working



* tests, docs



* link to enhancement request



* handle error



* Update docs/operator-manual/applicationset/Controlling-Resource-Modification.md




* fix bug, fix docs



* fix docs



* normalize empty syncPolicy field



---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
@vl-kp
Copy link

vl-kp commented Nov 14, 2023

When will 2.9.1 release?

@crenshaw-dev
Copy link
Member Author

@vl-kp a couple hours ago!

vladfr pushed a commit to vladfr/argo-cd that referenced this pull request Dec 13, 2023
* fix(appset): ignoreApplicationDifferences not working

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

* tests, docs

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

* link to enhancement request

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

* handle error

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

* Update docs/operator-manual/applicationset/Controlling-Resource-Modification.md

Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix bug, fix docs

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

* fix docs

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

* normalize empty syncPolicy field

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

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this pull request Dec 16, 2023
* fix(appset): ignoreApplicationDifferences not working

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

* tests, docs

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

* link to enhancement request

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

* handle error

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

* Update docs/operator-manual/applicationset/Controlling-Resource-Modification.md

Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix bug, fix docs

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

* fix docs

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

* normalize empty syncPolicy field

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

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
lyda pushed a commit to lyda/argo-cd that referenced this pull request Mar 28, 2024
* fix(appset): ignoreApplicationDifferences not working

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

* tests, docs

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

* link to enhancement request

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

* handle error

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

* Update docs/operator-manual/applicationset/Controlling-Resource-Modification.md

Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix bug, fix docs

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

* fix docs

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

* normalize empty syncPolicy field

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

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Signed-off-by: Kevin Lyda <kevin@lyda.ie>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this pull request Jun 16, 2024
* fix(appset): ignoreApplicationDifferences not working

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

* tests, docs

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

* link to enhancement request

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

* handle error

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

* Update docs/operator-manual/applicationset/Controlling-Resource-Modification.md

Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix bug, fix docs

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

* fix docs

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

* normalize empty syncPolicy field

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

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Hugues Peccatte <hugues.peccatte@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants