-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
⚠️ in-place propagation from MD to MS #8110
⚠️ in-place propagation from MD to MS #8110
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.
Am a little confused about the desired UX and backwards compatibility story on this one... it's possible I'm missing something
08c199e
to
e926767
Compare
/test pull-cluster-api-e2e-full-main |
e926767
to
730e6b6
Compare
/test pull-cluster-api-e2e-full-main |
internal/controllers/machinedeployment/machinedeployment_controller_test.go
Show resolved
Hide resolved
internal/controllers/machinedeployment/machinedeployment_controller_test.go
Outdated
Show resolved
Hide resolved
internal/controllers/machinedeployment/machinedeployment_sync_test.go
Outdated
Show resolved
Hide resolved
730e6b6
to
c6618d7
Compare
Thx, great work! /lgtm |
LGTM label has been added. Git tree hash: 4a2f9121843dda66e693be9154a027f99bddcd84
|
/assign @fabriziopandini |
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.
Great job @ykakarap ! LGTM from my side
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!
Are you planning on squashing commits before merge?
Yes. Looks like everyone reviewed the modified machine template hash logic. I will squash it. |
c6618d7
to
8e06d16
Compare
Commits are squashed. |
/lgtm |
Special thanks to @sbueringer and @fabriziopandini for helping with brainstorming and the regular feedback 🙏🏼 |
Great work!! Not exactly the most trivial part of our code base ;) |
/test pull-cluster-api-test-main |
/test pull-cluster-api-e2e-full-main |
/test pull-cluster-api-test-main Looks like a flake. Test unrelated to this PR failed. |
/retest Yup and all of them just recently added in other prs. Looks like we have some new flakes to fix |
Great work @ykakarap and thanks everyone for your valuable feedback! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR implements the in-place propagation behavior for MachineDeployments.
The PR contains the following features:
md.spec.template.metadata.labels
without triggering a rollout and the changes will be passed down to the target MachineSet.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Part of #7731