-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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: Appcontroller respects sync windows #16492
fix: Appcontroller respects sync windows #16492
Conversation
Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #16492 +/- ##
==========================================
- Coverage 49.73% 49.51% -0.22%
==========================================
Files 274 271 -3
Lines 48948 47805 -1143
==========================================
- Hits 24343 23670 -673
+ Misses 22230 21796 -434
+ Partials 2375 2339 -36 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com>
Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com>
f77b880
to
b067325
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
* fix: Appcontroller keeps op running when denied by sync window Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> * fix: Update test name Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> --------- Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com> Signed-off-by: Kevin Lyda <kevin@lyda.ie>
* fix: Appcontroller keeps op running when denied by sync window Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> * fix: Update test name Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> --------- Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
* fix: Appcontroller keeps op running when denied by sync window Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> * fix: Update test name Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> --------- Signed-off-by: Charles Coupal-Jetté <charles.coupaljette@goto.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
Closes #11817
Fix rationale
I have followed this hint to guide the solution choice.
I have found that this is the easiest way to solve the current problem, which is to simply keep the operation running while the sync window is in effect. Otherwise, I feel like a new
Suspended
orPending
OperationState would be needed to properly reflect the interaction with the SyncWindow, and it would have warranted a bigger re-work.In the tests I have made, failing the operation created a loop with the ApplicationSet controller, because it rescheduled the same operation all the time. This would also have required a bigger re-work in the ApplicationSet controller logic.
This is my first PR so please do not hesitate to tell me if you see problems related to this.
Checklist: