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.

Sign up for GitHub

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

Jump to bottom

fix(controller): use the stableRS from the rollout context rather tha… #3664

Merged
merged 4 commits into from
Aug 6, 2024
Merged

fix(controller): use the stableRS from the rollout context rather tha… #3664

merged 4 commits into from
Aug 6, 2024

Conversation

benminter-treatwell
Copy link
Contributor

@benminter-treatwell benminter-treatwell commented Jun 24, 2024 •
edited
Loading

…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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 24, 2024 •
edited
Loading

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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 24, 2024 •
edited
Loading

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
@codecov Codecov
Copy link

codecov bot commented Jun 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.32%. Comparing base (0ca5932) to head (29ac6ae).
Report is 70 commits behind head on master.

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
agrawroh
agrawroh reviewed Jul 4, 2024
View reviewed changes
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
agrawroh
agrawroh approved these changes Jul 9, 2024
View reviewed changes
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
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 26, 2024 •
edited
Loading

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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 26, 2024 •
edited
Loading

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!

@benminter-treatwell @zachaller
fix(controller): use the stableRS from the rollout context rather tha…
d3d2e59 
…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 <ben.minter@treatwell.com>
@benminter-treatwell @zachaller
fix(controller): update tests which were relying on this bug(?)
cf7bf9b 
Signed-off-by: ben.minter <ben.minter@treatwell.com>
@benminter-treatwell @zachaller
fix(controller): add clarity to comment in the case there is no stableRS
92efb33 
Signed-off-by: ben.minter <ben.minter@treatwell.com>
@benminter-treatwell @zachaller
fix(controller): add a test to assert that the stablers is not scaled…
29ac6ae 
… by the reconiliation on start, by checking the log

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

sonarcloud bot commented Aug 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
14.3% Duplication on New Code

See analysis details on SonarCloud

@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
@benminter-treatwell @zachaller
fix(controller): use the stableRS from the rollout context rather tha… (
c0db627 
#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 <ben.minter@treatwell.com>

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

Signed-off-by: ben.minter <ben.minter@treatwell.com>

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

Signed-off-by: ben.minter <ben.minter@treatwell.com>

* 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 <ben.minter@treatwell.com>

---------

Signed-off-by: ben.minter <ben.minter@treatwell.com>
@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
Reviewers

@agrawroh agrawroh agrawroh approved these changes

@zachaller zachaller zachaller approved these changes

Assignees

@zachaller zachaller

Labels
cherry-pick/release-1.7 cherry-pick-completed Used once we have cherry picked the PR to all requested releases
Projects
None yet
Milestone
No milestone
3 participants
@benminter-treatwell @zachaller @agrawroh