-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
[autoscaler] Translate to/from AWS 'Name' tag #2219
Conversation
Looks good to me. This makes me wonder if it might be, after all, better to use provider specific names for the tags. Having one condition here is not a problem, but who knows, maybe in future we end up having multiple similar cases where we have to do something provider specific configurations. |
Test FAILed. |
It's a fair point, if I knew about this edge case that would definitely have been the preferable option before... |
@@ -59,7 +78,7 @@ def node_tags(self, node_id): | |||
tags = {} | |||
for tag in node.tags: | |||
tags[tag["Key"]] = tag["Value"] | |||
return tags | |||
return from_aws_format(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.
why do we need this for this method in particular? ie, why not have this for all methods where you call to_aws_format
?
tags
is a dict and is passed by ref; so every time you call to_aws_format
you end up modifying the original...
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 the only one that returns tags?
And yes, the mutation is fine.
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.
lgtm
Tests look unrelated. |
What do these changes do?
GCP support moved tags to have cross-cloud names (#2061), however we should still use "Name" for the node name. GCP doesn't support upper-case tags, so implement this translation in the AWS node provider.
fyi @hartikainen
Related issue number
#2209