-
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
[0.1] Delete machines #1180
[0.1] Delete machines #1180
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.
Looks good so far! Few comments
2e61dc8
to
ca9ee02
Compare
/assign @ncdc |
Currently reviewing. /hold pending comments |
/test pull-cluster-api-test |
2 similar comments
/test pull-cluster-api-test |
/test pull-cluster-api-test |
well it passes now 🤷♀️ |
for _, child := range children { | ||
accessor, err := meta.Accessor(child) | ||
if err != nil { | ||
return reconcile.Result{}, errors.Wrapf(err, "couldn't create accessor for %T", child) |
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.
I agree that Accessor is unlikely to fail, but I'd still like to include it in errList instead of returning early.
children, err := r.listChildren(context.Background(), cluster) | ||
if err != nil { | ||
klog.Infof("Failed to list dependent objects of cluster %s/%s: %v", cluster.ObjectMeta.Namespace, cluster.ObjectMeta.Name, err) | ||
return reconcile.Result{}, errors.WithStack(err) |
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.
I don't think we need WithStack and instead we can return err directly because r.listChildren is our function, and it properly wraps/stacks all the errors it returns. WDYT?
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.
I'll be honest, I have no idea what WithStack does, I added it because you suggested it a few blocks up
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.
I guess you only need to add it once?
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.
By default, errors in Go do not include any stack trace information. Without it, it makes it very difficult to determine the source of an error (i.e. source code file & line), especially if the same error can be created in multiple places. github.com/pkg/errors makes it much easier to embed that stack information, and it includes a special custom format printer to print out the stack trace information.
The general guidance I've used in the past is that every line of code you write in a project that returns an error must either already have stack info, or you must wrap it. For first class functions (those defined in your project), you can/should assume that any errors they return are already wrapped. When you are invoking functions from vendored libraries, you can either check to see if they use github.com/pkg/errors, or you can just wrap.
57fabbe
to
e6d4114
Compare
1 last comment, then LGTM. I'd like 1 more review: |
Mostly nits, which can be fixed later. /lgtm Feel free to unhold when ready |
Co-Authored-By: Andy Goldstein <goldsteina@vmware.com>
/lgtm @liztio please remove the hold after you retest against CAPA. |
|
/hold cancel |
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: liztio, ncdc, 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:
Some pieces of cluster infrastructure (i.e. VPCs for CAPA) cannot be successfully deleted until after after the machines in the cluster are all deleted. This pull requests blocks cluster deletion until all owned resources are deleted.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #985 but it likely won't fix it until/unless we fix #1190
Special notes for your reviewer:
This only covers Machines, MachineSets, and MachineDeployments
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: