fix(app): Fix patch creation for Applications with ValuesObject fields #15126

[PaulSonOfLars]

Fix patch creation for Applications using ValuesObject fields, and add multiple tests to validate that the fix works as expected.

Tried to cherry pick and retain commit attribution @vladfr, but that might not have worked out well. Feel free happy to rebase onto a cleaner git history if you prefer, my intention wasnt to take ownership - credits go to your fix.

Extends and tests #15227, Fixes #15126

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[codecov]

Codecov Report

Attention: Patch coverage is 81.70732% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 49.29%. Comparing base (f87897c) to head (daab1c1).
Report is 114 commits behind head on master.

❗ Current head daab1c1 differs from pull request most recent head 08bfca1. Consider uploading reports for the commit 08bfca1 to get more accurate results

Files	Patch %	Lines
controller/appcontroller.go	81.70%	10 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #16602      +/-   ##
==========================================
- Coverage   49.73%   49.29%   -0.45%     
==========================================
  Files         274      274              
  Lines       48948    48192     -756     
==========================================
- Hits        24343    23754     -589     
+ Misses      22230    22088     -142     
+ Partials     2375     2350      -25     

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

[FredrikAugust]

Are there any updates on this? If this resolves the infinite refresh bug with valuesObject is would be greatly appreciated if it was merged 🙏

[blakepettersson]

@PaulSonOfLars I don't have any specific comments about the code itself, but could you add a few e2e tests to verify that the patching of valuesObject works as intended? Specifically the parts in regards to the infinite refresh.

[PaulSonOfLars]

Hi @blakepettersson, thanks for your comment! I've gone ahead and added an E2E test as requested.

I chose to add the test on application sets, since they deal with apps, as well as updating apps - happy to add a more specific application-only one if you'd like (or if theres anything else?). Took me a while to get familiar with the testsuite but I got there in the end!

Also found a cleaner refactor for the merge patch along the way which covers more cases and is easier to extend, so worked out well.

[mgledi]

@blakepettersson This problem is becoming more and more severe in our production system. It would be awesome if it is merged and released soon.

[eagleusb]

@blakepettersson This problem is becoming more and more severe in our production system. It would be awesome if it is merged and released soon.

It's the same for us, I was thinking it would be merged earlier (i.e. not forking the project) as it breaks probably a lot of applications.

[PaulSonOfLars]

I'm looking to add one more test (hopefully next week) as a sanity check, and then this should be ready to merge from my perspective.

If I could get a review before then, I'll make sure to address any comments at the same time as the test - @blakepettersson, @crenshaw-dev?
Also, if there's anything I should focus on, or anything I can do to help speed up reviews, just let me know!

[vladfr]

@PaulSonOfLars really nice job on this!

[KAllan357]

Any updates on this getting merged?

[7onn]

Please give us this fix 🙏🏼

[rsnyman]

I just ran into this issue today, and it's blocking a deployment. I would love to see this merged.

[hshepherd]

Thank you for the fix! I am seeing this issue as well. Looking forward this being released :)

[tamsky]

LGTM.

@argoproj/argocd-approvers ?

[crenshaw-dev]

I'm not fundamentally opposed to the hackaround, but since we apparently made a mistake by using the RawExtension type in the first place I want to give @PaulSonOfLars time to investigate alternatives before we merge a bunch of hack code. Paul, what are your thoughts? Merge this and fix the underlying issue later, or wait a bit to investigate some more?

[eyearian]

Can we get this merged?

[lexfrei]

@crenshaw-dev @PaulSonOfLars any news/eta on this?

[PaulSonOfLars]

@crenshaw-dev Alright, after various attempts to move away from the runtime.RawExtension type, I'm going to have to admit defeat here. As it stands, strategicpatch simply does not support the unstructured data that valuesObject requires.

I'd recommend merging this PR for now, and we can do the move in the future if someone figure it out.
I'll also add that if we chose to add other unstructured fields in the future (eg, kustomize patches?), the methods changed in this PR will need updating to include those too. I'll leave my other branches available (can see some attempts in #17725) in case someone tries to pick this up again in the future.

For the curious: It comes down to the fact that all strategicpatch merges expect to be done against a typed struct, and a freeform YAML/JSON object can't have that (by definition!). If the upstream k8s libraries exposed an option to fallback to standard json merge patches when no struct was found (or we hit a json.RawMessage type?), we might be able to use that; that would likely be the best path forward.

[crenshaw-dev]

Cool, let's ship it. I'm super hesitant to ship this with 2.11 since it's a change in such a core piece of code. I'm going to hold it for 2.12.

[alexmt]

I want to propose another way to fix this: #18061

The main reason IMHO it is safer. Less code changes and applicable only to valuesObject:

Another reason is I think the changes in valuesObject should be handled the same as changes in values which is replaced. The valuesObject was introduced as a convenience for values to avoid nested YAML/JSON. It would be strange to use merge for one and replace for another.

[jessesuen]

Adding my rejection here since I see that auto-merge is already enabled for this PR, but the current thinking is to do this alternatively with: #18061

[phyzical]

just thought id add, i just started running into this. was booleans as investigated. and the fix referenced worked for me a soon as i updated
👍

[blakepettersson]

Closing since this has been addressed with #18061 and #18515. Thanks for the work put in!