fix(controller): Fix for rollouts getting stuck in loop

[zachaller]

fixes: #2522

#2522 (comment)

[github-actions]

Go Published Test Results

1 948 tests   1 948 ✔️  2m 36s ⏱️
   118 suites         0 💤
       1 files           0 ❌

Results for commit fa42e3d.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (cc78ac5) 81.47% compared to head (fa42e3d) 81.48%.

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           

Impacted Files	Coverage Δ
rollout/controller.go	81.74% <100.00%> (+0.15%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[github-actions]

E2E Tests Published Test Results

    4 files      4 suites   3h 26m 29s ⏱️
  96 tests   85 ✔️   5 💤 6 ❌
390 runs  364 ✔️ 20 💤 6 ❌

For more details on these failures, see this check.

Results for commit fa42e3d.

♻️ This comment has been updated with latest results.

[agrawroh]

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.

[zachaller]

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.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
3.5% Duplication

[leoluz]

LGTM