Skip to content
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

Additional tags to ENI #734

Merged
merged 1 commit into from
Nov 27, 2019
Merged

Additional tags to ENI #734

merged 1 commit into from
Nov 27, 2019

Conversation

nithu0115
Copy link
Contributor

@nithu0115 nithu0115 commented Nov 26, 2019

Issue #, if available:
#643

Description of changes:

This feature allows you to add additional tags to ENIs created. Metadata applied to ENI help you categorize and organize your resources for billing or other purposes. Each tag consists of a custom-defined key and an optional value. Tag keys can have a maximum character length of 128 characters. Tag values can have a maximum length of 256 characters.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @nithu0115! Couple things to address inline...

README.md Show resolved Hide resolved
pkg/awsutils/awsutils.go Outdated Show resolved Hide resolved
pkg/awsutils/awsutils.go Outdated Show resolved Hide resolved
pkg/awsutils/awsutils.go Outdated Show resolved Hide resolved
@jaypipes
Copy link
Contributor

@nithu0115 please be aware of #726. If that merges first, you'll need to rebase and fix conflicts.

@nithu0115
Copy link
Contributor Author

Thanks for the review @jaypipes . Added changes as your comments.

Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, @nithu0115 :)

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

pkg/awsutils/awsutils.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! 🎉 👍

@mogren mogren merged commit 067ddf2 into aws:master Nov 27, 2019
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this @nithu0115 :)


Metadata applied to ENI help you categorize and organize your resources for billing or other purposes. Each tag consists of a custom-defined key and an optional value. Tag keys can have a maximum character length of 128 characters. Tag values can have a maximum length of 256 characters. These tags will be added to all ENIs on the host.

Important: Custom tags should not contain `k8s.amazonaws.com` prefix as it is reserved. If the tag has `k8s.amazonaws.com` string, tag addition will ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tagKey2 := "tagKey2"
tagValue1 := "tagValue1"
tagValue2 := "tagValue2"
tagKey3 := "cluster.k8s.amazonaws.com/name"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, you added a test that this one is stripped.


for key, value := range tagsMap {
keyPrefix := reservedTagKeyPrefix
if strings.Contains(key, keyPrefix) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of misleading (same goes for the error message below) as this is not a prefix check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants