-
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
🏃 Remove MachineSet controller Update fallback #2913
🏃 Remove MachineSet controller Update fallback #2913
Conversation
By marking it as optional and omitempty it won't be submitted to the server, we can just rely on it having a default of 0 by virtue of being a value type
/milestone v0.3.4 |
@benmoss Have you run some manual tests with tilt and/or clusterctl to double-check existing functionality isn't breaking after this change? More specifically:
MachineDeployment controller tests also exercise MachineSet, which is why I'm suggesting using a MD. |
Will do |
Everything seems fine with scale up / scale down / upgrade rollouts 👍 |
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benmoss, vincepri 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:
Removes some somewhat crufty logic around falling back to using
Update
whenPatch
fails on MachineSets due to validations. It seems that the patch that is constructed will never containreplicas: 0
, so it fails validation because it was a required field.By marking it as optional and omitempty it won't be submitted to the server, we can just rely on it having a default of 0 by virtue of being a Go value type.
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 #1945
/assign @vincepri