aws_ssm connection - Move connection vars environment handling into options #514
Conversation
ansibullbot
added
bug
There was a problem hiding this comment.
Thanks for this patch @dekimsey. Could you please add a changelog fragment (https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to)? It will also need to be rebased to address the git conflict.
ansibullbot
added
needs_rebase
tremble
changed the title
|
In my opinion this PR mix too much things, and use native AWS sdk in documentation is probably a bad idea. Ansible should use it's own variable to avoid scope conflicts. As exemple, we can imagine having different credentials for SSM connection and the bucket used for file transfers. Anyway, some vars are already used in this SSM plugin so this PR can be probably abandoned in the current state from march. |
|
My bad, I missunderstood some changes. Most of them are very useful. |
tremble
force-pushed
the
343-fix-connection-vars-handling
branch
from
feda6f9
to
3bf7bcf
Compare
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
ansibullbot
added
community_review
and removed
needs_rebase
This comment was marked as resolved.
This comment was marked as resolved.
tremble
force-pushed
the
343-fix-connection-vars-handling
branch
from
3bf7bcf
to
7aace6d
Compare
|
Many thanks for taking the time to submit this PR. Sorry for the delay getting this merged. I've rebased this PR and pruned it right back to "Just what it says on the tin". Including not trying to get clever with "placement". While it's true that this is re-using the AWS environment variables, that's what the module's always done (and what we usually do), and I don't think dropping those variables would be a good idea. |
tremble
added
mergeit
Backport to stable-5: 💚 backport PR created✅ Backport PR branch: Backported as #1683 🤖 @patchback |
aws_ssm connection - Move connection vars environment handling into options SUMMARY This fix moves a number of connection related variables to the options parsing step instead of inline. This has the added effect of documenting their existence and making overriding them more consistent with Ansible's UX. Fixes #343 ISSUE TYPE Bugfix Pull Request COMPONENT NAME aws_ssm ADDITIONAL INFORMATION There were a couple of other minor changes related to logging and silencing curl's progress info outside of the connection vars themselves. I'm happy to pull them out if desired and submit them as a separate PR. I added fallback on hostnames lookup to match SSH's host handling since that's the defacto connection plugin. This incidentally fixes the way delegation is reported (it didn't show the -> delegated host bit in the logs). Of note, the ec2.py module sets the instance_id and placement on instances it detects so I added it as first-class fallback for instance_id and region parameters respectively. The get_options parser doesn't handle nested variable lookups, so I had to modify the lookup slightly. Reviewed-by: Jill R <None> Reviewed-by: Guillaume GILL <None> Reviewed-by: Mark Chappell <None> (cherry picked from commit 94d1295)
) [PR #514/94d12952 backport][stable-5] aws_ssm connection - Move connection vars environment handling into options This is a backport of PR #514 as merged into main (94d1295). SUMMARY This fix moves a number of connection related variables to the options parsing step instead of inline. This has the added effect of documenting their existence and making overriding them more consistent with Ansible's UX. Fixes #343 ISSUE TYPE Bugfix Pull Request COMPONENT NAME aws_ssm ADDITIONAL INFORMATION There were a couple of other minor changes related to logging and silencing curl's progress info outside of the connection vars themselves. I'm happy to pull them out if desired and submit them as a separate PR. I added fallback on hostnames lookup to match SSH's host handling since that's the defacto connection plugin. This incidentally fixes the way delegation is reported (it didn't show the -> delegated host bit in the logs). Of note, the ec2.py module sets the instance_id and placement on instances it detects so I added it as first-class fallback for instance_id and region parameters respectively. The get_options parser doesn't handle nested variable lookups, so I had to modify the lookup slightly. Reviewed-by: Mark Chappell <None>
SUMMARY
This fix moves a number of connection related variables to the options parsing step instead of inline. This has the added effect of documenting their existence and making overriding them more consistent with Ansible's UX.
Fixes #343
ISSUE TYPE
COMPONENT NAME
aws_ssm
ADDITIONAL INFORMATION
There were a couple of other minor changes related to logging and silencing curl's progress info outside of the connection vars themselves. I'm happy to pull them out if desired and submit them as a separate PR.
I added fallback on hostnames lookup to match SSH's host handling since that's the defacto connection plugin. This incidentally fixes the way delegation is reported (it didn't show the
-> delegated hostbit in the logs).Of note, the ec2.py module sets the
instance_idandplacementon instances it detects so I added it as first-class fallback for instance_id and region parameters respectively. Theget_optionsparser doesn't handle nested variable lookups, so I had to modify the lookup slightly.