-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix(applicationset_controller): Change appset parameter to pointer in performProgressiveSyncs #22951
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
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
… performProgressiveSyncs Signed-off-by: Dmitry Shmelev <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22951 +/- ##
==========================================
- Coverage 60.03% 60.00% -0.04%
==========================================
Files 344 344
Lines 57781 57787 +6
==========================================
- Hits 34689 34673 -16
- Misses 20333 20350 +17
- Partials 2759 2764 +5 ☔ View full report in Codecov by Sentry. |
|
Can we add a unit test that demonstrates the change? Eg, before your changes, it shows that status is not persisted on the applicationset that is passed down, and afterwards it is. |
@rumstead Let's clarify first. What should we do when the Upd: This is the second solution: dshmelev@83e1c4a |
Should we handle it the same way that we do for applications? Add a condition for when progressive syncs are enabled? |
Hi folks 👋
This PR fixes an inconsistency in
ProgressiveSync.Currently,
performProgressiveSyncsupdates the application status viaupdateApplicationSetApplicationStatusProgress, and then callssetAppSetApplicationStatus. However, sinceperformProgressiveSyncsreceivesapplicationSetInfoby value, the updated status is not visible tosyncValidApplications, which uses a different (stale) copy.This leads to incorrect behavior where sync decisions are made based on outdated status information.
Fix: Change
performProgressiveSyncsandsyncValidApplicationsto accept*argov1alpha1.ApplicationSetso that they share the same state, and changes to application statuses are properly propagated.Let me know what you think — happy to adjust if needed!
Issue: #22558
Discussion: https://cloud-native.slack.com/archives/C01U45M2SVB/p1746622854063139
Checklist: