-
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
🐛Add DeleteMachineAnnotation for MachineSet Delete Policy #2929
🐛Add DeleteMachineAnnotation for MachineSet Delete Policy #2929
Conversation
Code changes look good, deprecation warning should be there as @benmoss suggested /milestone v0.3.4 |
LGTM for me as well. |
While we're in here - the code currently requires a non-empty string for the annotation's value. Should we change to a presence check? |
+1 from me |
Thanks for bringing this up. I thought that was weird too and wanted to bring that up. I'll fix it to make it a presence check instead. |
9645a74
to
ffb2640
Compare
ffb2640
to
32a4862
Compare
Changes LGTM |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri, wfernandes 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 |
@vincepri Do we have an issue created to remove |
Yes please, we can put it in v0.4 |
/lgtm |
What this PR does / why we need it:
This PR adds another annotation called
DeleteMachineAnnotation=cluster.x-k8s.io/delete-machine
to the MachineSet Delete Policy because that annotation is the one that is documented.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 #2912
Release Notes
controllers.DeleteMachineAnnotation
with valuecluster.x-k8s.io/delete-machine
. Please use this annotation moving forward as the other annotationDeleteNodeAnnotation
is planned for removal in the next CAPI release.