Use CreateTags instead of TagResources#129
Conversation
liwenwu-amazon
left a comment
liwenwu-amazon
left a comment
There was a problem hiding this comment.
Can you add how you test your changes? thanks
vsiddharth
left a comment
vsiddharth
left a comment
There was a problem hiding this comment.
Overall changes look good. Left minor suggestions.
Could you please add some tests to validate the new changes?
| reflect "reflect" | ||
|
|
||
| "github.com/aws/aws-sdk-go/service/ec2" | ||
| ec2 "github.com/aws/aws-sdk-go/service/ec2" |
There was a problem hiding this comment.
Was this autogenerated using a newer version of mockgen?
There was a problem hiding this comment.
Seems like it, due to the slight differences. It was generated with go generate ./pkg/awsutils/...
pkg/awsutils/awsutils.go
Outdated
| tags[eniTagKey] = aws.String(tagValue) | ||
| log.Debugf("Trying to tag newly created eni: keys=%s, value=%s", eniTagKey, tagValue) | ||
| for _, tag := range tags { | ||
| log.Debugf("Trying to tag newly created eni: key=%s, value=%s", *tag.Key, *tag.Value) |
There was a problem hiding this comment.
Can you use aws.StringValue here?
pkg/awsutils/awsutils.go
Outdated
| awsAPIErrInc("TagResources", err) | ||
| log.Warnf("Fail to tag the newly created eni %s %v", eniID, err) | ||
| awsAPIErrInc("CreateTags", err) | ||
| log.Warnf("Fail to tag the newly created eni %s: %v", eniID, err) |
There was a problem hiding this comment.
Good catch, done.
| Resource: "network-interface/" + eniID} | ||
| arnString := eniARN.String() | ||
| arns = append(arns, aws.String(arnString)) | ||
| tags := []*ec2.Tag{ |
There was a problem hiding this comment.
Can you add a comment with the sample tag keys and values for an ENI??
pkg/awsutils/awsutils.go
Outdated
| log.Warnf("Fail to tag the newly created eni %s: %v", eniID, err) | ||
| } else { | ||
| log.Debugf("Tag the newly created eni with arn: %s", arnString) | ||
| log.Debugf("Tag the newly created eni: %s", eniID) |
There was a problem hiding this comment.
Can you please rephrase this message to
"Successfully tagged eni: %s", eniID
There was a problem hiding this comment.
Yup, makes more sense.
* Also introduce two new tags: - node.k8s.amazonaws.com/instance_id - cluster.k8s.amazonaws.com/name (only tagged if the env var CLUSTER_NAME is present).
nckturner
force-pushed
the
move-to-create-tags
branch
from
9d40633 to
c82dc6e
Compare
3 participants
Issue #, if available:
Fixes: #76
Description of changes:
Testing:
Created an EKS cluster with a node, which was created with the current CNI. I checked the tag on the ENI, it was the old format. I then applied CNI yaml with a container I built and scaled in and out the autoscaling group to get new nodes. The first time I did this with CLUSTER_NAME set:
Second time without:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.