-
Notifications
You must be signed in to change notification settings - Fork 873
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(controller): Fix for rollouts getting stuck in loop #2689
fix(controller): Fix for rollouts getting stuck in loop #2689
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2689 +/- ##
=======================================
Coverage 81.47% 81.48%
=======================================
Files 133 133
Lines 20154 20159 +5
=======================================
+ Hits 16421 16426 +5
Misses 2881 2881
Partials 852 852
☔ View full report in Codecov by Sentry. |
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.
Do we have enough coverage around this? Can we maybe add a few Unit/Integration tests? It'd be good to see if we can repro the behavior seen in the linked ticket without this fix and show that it's no longer an issue with this patch.
It looks like the right thing to do but I'm not sure if we'll end up uncovering some other subtle bug by changing this order.
I want to try to add tests around this I just am not sure I will be able to the unit test it, fakeclient is not accurate enough for unit tests to work possibly, and I am worried about how to simulate it in e2e but I plan on trying both especially if I can get some confirmation that it does in deed seem to fix the issue. I agree though that getting a reproducible test would be ideal. I did hack together some code to test this locally but it involved editing rollouts controller to do a double update. |
Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
Signed-off-by: zachaller <[email protected]>
5c1b63f
to
fa42e3d
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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
* possible fix for sutck rollouts Signed-off-by: zachaller <[email protected]> * add comments Signed-off-by: zachaller <[email protected]> * add comments Signed-off-by: zachaller <[email protected]> * update comments Signed-off-by: zachaller <[email protected]> --------- Signed-off-by: zachaller <[email protected]>
fixes: #2522
#2522 (comment)