-
Notifications
You must be signed in to change notification settings - Fork 403
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
aws_ssm: add profile support in connections #278
aws_ssm: add profile support in connections #278
Conversation
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.
Cannot judge whether the changes to _get_boto_client are good, but the rest looks good!
Added a minor fix to the integration test that should hopefully unbreak it - I was able to successfully run it with just that change. I did not enable the test because I'm not sure how this can account for the flakiness that was brought up in IRC. |
I've applied this change over the top of #295 and I'm consistently getting an exception:
I need to dig into this a little further to understand why, but it looks like something's not quite right with the change, possibly how it handles not being passed a profile (that's a wild guess). |
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.
I really appreciate you spending the time to track down the policy issue that was breaking the tests. For what it's worth, doing so appears to have uncovered a couple of small issues with the changes.
Hi @maxeaubrey, Can you please rebase your PR, this way we will be able to continue this review. Thank you for your contribution. |
Thanks, I've rebased - though admittedly I did not retest this myself as I won't be able to until Monday 😅 |
Hi @maxeaubrey, thank you for your contribution. There was one was task failing due to timeout, so I will rerun the CI. May I ask you to add a changeling fragment https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs? |
@alinabuzachis Happily! Pushed a commit for it, hope it's good. |
@@ -304,10 +309,10 @@ def start_session(self): | |||
raise AnsibleError("failed to find the executable specified %s." | |||
" Please verify if the executable exists and re-try." % executable) | |||
|
|||
profile_name = '' | |||
profile_name = self.get_option('profile') or '' |
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.
Could this be better set as an entry on the configuration blob above?
Perhaps with AWS_PROFILE support too?
profile:
...
default: ''
env:
- name: AWS_PROFILE
`cmd` in `start_session` needs `profile_name` to be set to a string we oblige by having a fallback when it's set to None, changing it to be an empty string instead
not all uses of _get_boto_client had a profile passed to them, now s3 sessions will also use the specified profile
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.
Tested locally
LGTM |
Hi @maxeaubrey, sorry it's taken so long to get this merged. The feature should be available once v1.5.0 of this collection is released (probably sometime this month) |
Thanks for your contribution |
@tremble Awesome! Thanks for helping me get this into a mergeable state! |
…n_profile_support aws_ssm: add profile support in connections Reviewed-by: https://github.com/apps/ansible-zuul
…n_profile_support aws_ssm: add profile support in connections Reviewed-by: https://github.com/apps/ansible-zuul
SUMMARY
The
aws_ssm
connection type had no way of setting a profile except via environment variable. We have a specific need to set the profile name for this connection type, and this achieves that.ISSUE TYPE
COMPONENT NAME
plugins/connection/aws_ssm.py