Include more information when multiple security groups are tagged #58874
Conversation
k8s-ci-robot
added
release-note
|
/assign @justinsb |
| return nil, fmt.Errorf("Multiple tagged security groups found for instance %s; ensure only the k8s security group is tagged", instanceID) | ||
| taggedGroups := "" | ||
| for _, v := range tagged { | ||
| taggedGroups += fmt.Sprintf("%v(%v) ", v.GroupId, v.GroupName) |
There was a problem hiding this comment.
I think these are *string so you should fmt.Printf("%s(%s) ", aws.StringValue(v.GroupId), aws.StringValue(v.GroupName))
| for _, v := range tagged { | ||
| taggedGroups += fmt.Sprintf("%v(%v) ", v.GroupId, v.GroupName) | ||
| } | ||
| return nil, fmt.Errorf("Multiple tagged security groups found for instance %s; ensure only the k8s security group is tagged; the tagged groups was %v", instanceID, taggedGroups) |
There was a problem hiding this comment.
nit (only because we need a change here anyway): the tagged groups were %v
|
Love the idea here, but I think we need to use |
sorenmat
force-pushed
the
sg_tag_verbose
branch
from
cb27142
to
927c8cf
Compare
k8s-ci-robot
added
size/S
|
@justinsb addressed your comments, also added a few tests for good measure |
|
/ok-to-test /lgtm Thanks @sorenmat |
k8s-ci-robot
added
lgtm
When trying to create ELB we can sometime fail if there is more then one AWS security group tagged. It very useful to get the list of security groups printed in the error message. **Release note**: ```release-note Include the list of security groups when failing with the errors that more then one is tagged ```
sorenmat
force-pushed
the
sg_tag_verbose
branch
from
927c8cf
to
7c7e691
Compare
k8s-github-robot
removed
the
lgtm
|
@justinsb I've updated the code so it should pass the steps |
|
/test pull-kubernetes-typecheck |
|
re-apply lgtm /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, justinsb, sorenmat 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 |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/test all Tests are more than 96 hours old. Re-running tests. |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
When trying to create ELB we can sometime fail if there is more then one AWS
security group tagged. It very useful to get the list of security groups printed in
the error message.
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 #
Special notes for your reviewer:
Release note: