-
Notifications
You must be signed in to change notification settings - Fork 35
Danbar/e2e rolling update 10 #356
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
Conversation
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.
Pull request overview
This PR enhances the rolling update functionality and fixes test infrastructure for e2e tests. The main focus is on fixing Test_RU10 to properly verify delete-first rolling update behavior when resources are insufficient, and improving Docker image handling in the test setup.
Changes:
- Fixed logic in
mutateUpdatedReplicato correctly track updated replicas when a rolling update completes - Updated Test_RU10 to verify delete-first strategy behavior with cordoned nodes
- Refactored Docker image pulling logic to avoid duplication and add authentication support
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| operator/internal/controller/podclique/reconcilestatus.go | Added handling for the window between rolling update completion and hash update |
| operator/e2e/tests/rolling_updates_test.go | Uncommented and fixed Test_RU9 and Test_RU10, updated timeouts and verification logic |
| operator/e2e/setup/shared_cluster.go | Added Docker authentication support and extracted common image pull logic |
| operator/e2e/setup/k8s_clusters.go | Refactored to use common image pull helper function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gflarity
left a comment
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.
Left some comments around the utils, but looks like this also addresses
#315. Nice!
74055b8 to
53c1b55
Compare
gflarity
left a comment
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.
Looking good, one question and one comment.
53c1b55 to
f44d01a
Compare
f44d01a to
ad88be8
Compare
|
@unmarshall please review this fix for rolling updates |
ad88be8 to
5d0c9d6
Compare
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a API change?
Additional documentation e.g., enhancement proposals, usage docs, etc.: