-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Delete old tags when cloudLabels / labels / taints are removed #3207
Delete old tags when cloudLabels / labels / taints are removed #3207
Conversation
Hi @KashifSaadat. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/assign @justinsb |
if tagsRequest != nil { | ||
_, err := t.Cloud.Autoscaling().CreateOrUpdateTags(tagsRequest) | ||
if updateTagsRequest != nil { | ||
_, err := t.Cloud.Autoscaling().CreateOrUpdateTags(updateTagsRequest) |
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.
might as well .. if _, err := ; err != nil here
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.
Will update, cheers.
if err != nil { | ||
return fmt.Errorf("error updating AutoscalingGroup tags: %v", err) | ||
} | ||
} | ||
|
||
if (deleteTagsRequest != nil) && (len(deleteTagsRequest.Tags) > 0) { | ||
_, err := t.Cloud.Autoscaling().DeleteTags(deleteTagsRequest) |
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.
same here ... if _, 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.
Updating..
if err != nil { | ||
return fmt.Errorf("error updating AutoscalingGroup tags: %v", err) | ||
} | ||
} | ||
|
||
if (deleteTagsRequest != nil) && (len(deleteTagsRequest.Tags) > 0) { |
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.
brackets aren't required
if changes.Tags != nil { | ||
tagsRequest = &autoscaling.CreateOrUpdateTagsInput{} | ||
tagsRequest.Tags = tags | ||
updateTagsRequest = &autoscaling.CreateOrUpdateTagsInput{} |
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.
&autoscaling.CreateOrUpdateTagsInput{Tag: tags}
@@ -149,7 +149,28 @@ func (_ *EBSVolume) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *EBSVolume) e | |||
e.ID = response.VolumeId | |||
} | |||
|
|||
return t.AddAWSTags(*e.ID, e.Tags) | |||
err := t.AddAWSTags(*e.ID, e.Tags) |
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.
if err := |thing|; err != nil
awesome 👍 |
f1e0892
to
eb2c947
Compare
@@ -27,6 +28,7 @@ import ( | |||
"github.com/aws/aws-sdk-go/service/iam" | |||
"github.com/aws/aws-sdk-go/service/route53/route53iface" | |||
"github.com/golang/glog" | |||
|
|||
"k8s.io/kops/pkg/apis/kops" | |||
"k8s.io/kops/upup/pkg/fi" | |||
"k8s.io/kubernetes/federation/pkg/dnsprovider" |
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.
this is third party
eb2c947
to
51cc3c8
Compare
} | ||
|
||
// TODO: Use PropagateAtLaunch = false for tagging? | ||
|
||
return nil // We have | ||
} | ||
|
||
// getASGTagsToDelete loops through the currently set tags and builds a list of |
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.
the comment is incorrect
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.
Updated function names now. :)
8b8b604
to
8261767
Compare
/ok-to-test |
@gambol99 since you reviewed I probably do not need to ... can I get a LGTM from you? |
8261767
to
eabbe44
Compare
…ted resources (cloudLabels, labels, taints).
eabbe44
to
1574b19
Compare
@chrislovecnm I've fixed the panic, just had to check that the |
lgtm @KashifSaadat ... |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KashifSaadat, chrislovecnm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
If you remove custom cloudLabels/labels/taints from the cluster configuration, kops does not correctly update the AWS resources to delete the tags. This seems to be because it only calls the AWS API method
CreateOrUpdateTags
, which won't remove tags that aren't in the supplied list.The current behaviour is that every
kops update cluster
will show a tag difference but never successfully apply the changes (remove the extra tags).This PR will perform a diff of the current and expected tags, and call the
DeleteTags
API if there are any tags to delete.