-
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
Fix #268 #269
Fix #268 #269
Conversation
@@ -69,7 +69,7 @@ class Ec2 < Kitchen::Driver::Base # rubocop:disable Metrics/ClassLength | |||
default_config :aws_access_key_id, nil | |||
default_config :aws_secret_access_key, nil | |||
default_config :aws_session_token, nil | |||
default_config :aws_ssh_key_id, ENV["AWS_SSH_KEY_ID"] | |||
default_config :aws_ssh_key_id, ENV["AWS_SSH_KEY_ID"] || nil |
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 || nil
here is redundant
besides the nit above. I think this is a good move but we need to fix the tests. |
Fixed tests and all succeed locally with 'chef exec rake test' (see below). Travis CI build is not working due to mismatch of dependencies in Travis build environment:
Output of local tests
|
Is there anything else I can do to facilitate acceptance of this PR? |
I just submited PR #272 which once merged should get this green. |
I reran the travis test after merging #272 and its green now so I'm 👍 |
Awesome! I don't see that LGTM quorum is required for this repo, but once this is merged, is there a Travis hook to publish to rubygems.org? |
So we need to fix this repo but all the kitchen-* plugins maintained by @tk-core require two +1s but I think we only have that on the core repo at the moment. +1 |
I trouble running the test cases, but added a new case for "minimum" ec2 instance (without aws_ssh_key_id specified).