-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azure: Add mode dependency logic to deletion #15617
azure: Add mode dependency logic to deletion #15617
Conversation
0374b96
to
344d5b3
Compare
/retest |
/kind office-hours |
/assign @justinsb |
pkg/resources/azure/azure.go
Outdated
var blocks []string | ||
blocks = append(blocks, toKey(typeResourceGroup, g.resourceGroupName())) | ||
|
||
nsgs := map[string]struct{}{} |
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.
Why not use a k8s.io/utils/set/set.Set[string]
?
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.
Interesting approach. Thanks.
/retest |
for _, lb := range *ip.LoadBalancerBackendAddressPools { | ||
lbID, err := azure.ParseLoadBalancerID(*lb.ID) | ||
if err != nil { | ||
return nil, fmt.Errorf("parsing load balancer ID: %s", 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.
Nit: should probably be ...: %w
(?)
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.
It's just copy/paste from the original code, wanted to be easy diff.
func ParseSubnetID(s string) (*SubnetID, error) { | ||
l := strings.Split(s, "/") | ||
if len(l) != 11 { | ||
return nil, fmt.Errorf("malformed format of subnet ID: %s, %d", s, len(l)) |
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.
Nit: I normally prefer to print with %q to highlight the "wrong" value, and I'm not sure that printing the length really adds any information (though I know it is handy during development!)
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.
It's just copy/paste from the original code, wanted to be easy diff.
Thanks @hakman /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb 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 |
/cc @justinsb @rifelpet