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

ec2: per-platform ansible_connection overrides and win password lookup #2271

Merged
merged 4 commits into from
Sep 11, 2019
Merged

ec2: per-platform ansible_connection overrides and win password lookup #2271

merged 4 commits into from
Sep 11, 2019

Conversation

troyready
Copy link
Contributor

@troyready troyready commented Sep 5, 2019

PR Type

  • Feature Pull Request

Overview

To provision Linux & Windows hosts simultaneously, the same connection_options can't be shared on the provisioner. This PR allows provisioner connection_options to be customized per-platform.

Additionally, if the winrm connection is used on an EC2 instance and a password isn't supplied. the password will be retrieved automatically via boto3 (decryption logic copied from ec2_win_password module)

Copy link
Contributor

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

Thanks for this. Haven't tested it but here's a review pass. Given that we're in the process of migrating our drivers out of the core and into plugins, I am unsure if we'll be merging this now or a bit later. Perhaps now is easier? /cc @ssbarnea

molecule/driver/ec2.py Outdated Show resolved Hide resolved
molecule/driver/ec2.py Outdated Show resolved Hide resolved
molecule/driver/ec2.py Show resolved Hide resolved
Copy link
Contributor

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

Looking good! Let's get a green CI then?

I think this just needs a rebase and then the CI will not complain.

To provision Linux & Windows hosts simultaneously, the same
connection_options can't be shared on the provisioner. This commit
allows provisioner connection_options to be customized per-platform.

Additionally, if the winrm connection is used on an EC2 instance and a
password isn't supplied. the password will be retried automatically via
boto3 (decryption logic copied from ec2_win_password module)

Signed-off-by: troyready <troy@troyready.com>
Signed-off-by: troyready <troy@troyready.com>
Signed-off-by: troyready <troy@troyready.com>
Signed-off-by: troyready <troy@troyready.com>
@troyready
Copy link
Contributor Author

Sure thing - fixed

Copy link
Contributor

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

This is looking fine then!

I would rather reduce the noise of chaning quotes from single / double but can let that pass for now.

Thanks for taking time to work on this @troyready 🚀

@troyready
Copy link
Contributor Author

I couldn't agree more about the quotes; I'd be happy to change them back (and have this PR only include relevant changes), but from what I could tell CI was failing because black was changing the file

(i.e. it was still failing after the rebase and the only way to get it to exit zero was to run the file through black)

@decentral1se
Copy link
Contributor

I couldn't agree more about the quotes; I'd be happy to change them back (and have this PR only include relevant changes), but from what I could tell CI was failing because black was changing the file

Ah, I see. The Black invocation should be picking up the standard configuration over in https://github.com/ansible/molecule/blob/master/pyproject.toml#L11 which avoids changing quotes. Invoking it through Tox should pick this all up automatically 😕

@ssbarnea ssbarnea self-requested a review September 11, 2019 16:05
@ssbarnea ssbarnea merged commit d57881f into ansible:master Sep 11, 2019
@ssbarnea ssbarnea added the bug label Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants