-
Notifications
You must be signed in to change notification settings - Fork 182
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
chore: cleanup deprovisioning types #289
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.
Really just one uber comment on command comparison
Pull Request Test Coverage Report for Build 4885735774
💛 - Coveralls |
Can we define a protocol for manual testing for any changes to the core deprovisioning logic? Further, can you elaborate in the description about why this change is being made? |
Overall I'm fine with this change, but I'm nervous given the recent regressions to deprovisioning. It's not an option to stop making changes, but we need to figure out what mechanisms we can add to increase our confidence in making changes. |
* Revert "Revert machine migration changes (kubernetes-sigs#176) (kubernetes-sigs#241)" This reverts commit 9973eac. * Change owner reference to blockOwnerDeletion * Remove string logging for machine launch * Removing FailedInit since machine statusCondition captures it * Updated eventing on machines
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 🚀
Fixes #280
Description
Removes an the old
action
type in the deprovisioning controller since the type of action can be inferred from the len(candidates) to replace or len(replacements).How was this change tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.