-
Notifications
You must be signed in to change notification settings - Fork 202
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
Don't break if we're using a custom "platform" AMI #273
Conversation
7326728
to
2cc9be6
Compare
+1 |
@@ -289,7 +289,8 @@ def default_ami | |||
def update_username(state) | |||
# TODO: if the user explicitly specified the transport's default username, | |||
# do NOT overwrite it! | |||
if instance.transport[:username] == instance.transport.class.defaults[:username] | |||
if actual_platform && | |||
instance.transport[:username] == instance.transport.class.defaults[:username] |
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.
is instance.transport.class.defaults[:username]
always nil? because this is a bit difficult to read and there's going to be a bug in here if that default is ever not nil... i'd prefer this to just read if actual_platform && !instance.transport[:username].nil?
it also looks like the TODO in the comment has been done?
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 bug is that if the user tried to set the username to the default where the username is not nil by default, then it'll go off and do the autodetection and may choose to override the default. this check for username != default cannot catch that case -- unless the default value is something like nil which the user would never set.
it feels like someone was thinking too hard about default values 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.
Neither of those instance.transport
s are Nil for me (it's actual_platform.username
that's coming up Nil when using non-"standard" AMI's, causing the error).
With a fresh setup and me not specifying anything, both of them are set to "root". I can alter instance.transport[:username]
by setting transport.username
in my .kitchen.yml, and i think the default value for it - and instance.transport.class.defaults[:username]
- are probably both coming from TK transport defaults? (ie; test-kitchen-1.13.0/lib/kitchen/transport/ssh.rb, which is "root")
I guess that's why that equality condition can't just check for nil?
and the TODO still applies... (if i want to keep my TK username as "root" and i'm using the SSH transport, it looks like it'll always be overwritten by the actual_platform.username
value?)
I didn't want to tinker with the existing username logic too much with just that comment for guidance, and if both of those instance.transport
variables are getting defaults, i'm not sure how you'd test if the user was overriding one? Maybe @jkeiser remembers a bit more about it?
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.
okay it'd be good to get that information documented in the TODO so we don't have to re-discover it all again at some point in the future. you might just rewrite the TODO more like "BUG: the default is often non-nil (e.g. 'root') but if the user specifies 'root' explicitly then we will overwrite that value -- need a way to distinguish a default from a set value"
its amusing how every configuration system inevitably re-discovers this problem...
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.
Reworded TODO to BUG with more info
If we're a non-standard platform, actual_platform isn't populated so the debug message tries to access the "username" method of a nil class.
Two thumbs! Up for merge and gem release? |
Many thanks for merging this. Is there an ETA on the gem release? |
If i'm using an private image_id whose AMI Name doesn't match one of the "standard_platform"s (ie; "oel-7.2"), i can't seem to override the username. Instead, i get the following error:
which - i think - is because actual_platform doesn't get populated, so update_username which relies on it fails?
Possibly the same thing that @gretel's comment refers to here
This PR works around it by not attempting to do anything with the username if actual_platform isn't defined (and therefore assumes you're setting the
transport.username
yourself).