Skip to content
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): use the stableRS from the rollout context rather tha… #3664

Conversation

benminter-treatwell
Copy link
Contributor

@benminter-treatwell benminter-treatwell commented Jun 24, 2024

…n inferring it from the active selector, to deal with the edge case where the stableRS != activeRS during analysis templates. Fixes #3663

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

Copy link
Contributor

github-actions bot commented Jun 24, 2024

Go Published Test Results

2 173 tests   2 173 ✅  2m 55s ⏱️
  119 suites      0 💤
    1 files        0 ❌

Results for commit e1692ff.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jun 24, 2024

E2E Tests Published Test Results

  4 files    4 suites   3h 26m 45s ⏱️
111 tests 102 ✅  6 💤 3 ❌
452 runs  421 ✅ 24 💤 7 ❌

For more details on these failures, see this check.

Results for commit e1692ff.

♻️ This comment has been updated with latest results.

@benminter-treatwell benminter-treatwell force-pushed the fix/post-promotion-scaling-events-cause-irreconcilable-rollout branch from c99fdac to 4e31b41 Compare June 24, 2024 15:04
Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.32%. Comparing base (0ca5932) to head (29ac6ae).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3664      +/-   ##
==========================================
+ Coverage   84.19%   84.32%   +0.13%     
==========================================
  Files         154      154              
  Lines       18025    18022       -3     
==========================================
+ Hits        15176    15197      +21     
+ Misses       2003     1990      -13     
+ Partials      846      835      -11     

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

@zachaller zachaller self-assigned this Jun 25, 2024
rollout/bluegreen.go Outdated Show resolved Hide resolved
@benminter-treatwell benminter-treatwell force-pushed the fix/post-promotion-scaling-events-cause-irreconcilable-rollout branch from b8030de to f06a8bf Compare July 8, 2024 14:13
Copy link
Member

@agrawroh agrawroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. @zachaller Could you do a final pass?

@benminter-treatwell benminter-treatwell force-pushed the fix/post-promotion-scaling-events-cause-irreconcilable-rollout branch from 754a16d to 459d9ac Compare July 16, 2024 15:15
@benminter-treatwell
Copy link
Contributor Author

Doing a rebase to keep this up to date and ready to go

@agrawroh agrawroh requested a review from zachaller July 25, 2024 09:10
Copy link
Contributor

github-actions bot commented Jul 26, 2024

Published E2E Test Results

  4 files    4 suites   3h 24m 32s ⏱️
111 tests 100 ✅  6 💤 5 ❌
450 runs  421 ✅ 24 💤 5 ❌

For more details on these failures, see this check.

Results for commit 29ac6ae.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jul 26, 2024

Published Unit Test Results

2 176 tests   2 176 ✅  2m 55s ⏱️
  119 suites      0 💤
    1 files        0 ❌

Results for commit 29ac6ae.

♻️ This comment has been updated with latest results.

@benminter-treatwell
Copy link
Contributor Author

@zachaller dropping a nudge here, would be awesome to get this merged!

…n inferring it from the active selector, to deal with the edge case where the stableRS != activeRS during analysis templates

Signed-off-by: ben.minter <[email protected]>
… by the reconiliation on start, by checking the log

Signed-off-by: ben.minter <[email protected]>
@zachaller zachaller force-pushed the fix/post-promotion-scaling-events-cause-irreconcilable-rollout branch from c2667ba to 29ac6ae Compare August 6, 2024 15:00
Copy link

sonarcloud bot commented Aug 6, 2024

@zachaller zachaller merged commit 827ce59 into argoproj:master Aug 6, 2024
25 checks passed
zachaller pushed a commit that referenced this pull request Aug 13, 2024
#3664)

* fix(controller): use the stableRS from the rollout context rather than inferring it from the active selector, to deal with the edge case where the stableRS != activeRS during analysis templates

Signed-off-by: ben.minter <[email protected]>

* fix(controller): update tests which were relying on this bug(?)

Signed-off-by: ben.minter <[email protected]>

* fix(controller): add clarity to comment in the case there is no stableRS

Signed-off-by: ben.minter <[email protected]>

* fix(controller): add a test to assert that the stablers is not scaled by the reconiliation on start, by checking the log

Signed-off-by: ben.minter <[email protected]>

---------

Signed-off-by: ben.minter <[email protected]>
@zachaller zachaller added the cherry-pick-completed Used once we have cherry picked the PR to all requested releases label Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-1.7 cherry-pick-completed Used once we have cherry picked the PR to all requested releases
Projects
None yet
3 participants