community.aws icon indicating copy to clipboard operation
community.aws copied to clipboard

aws_ssm connection - Move connection vars environment handling into options

Open dekimsey opened this issue 4 years ago • 2 comments
trafficstars

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.

dekimsey avatar Mar 30 '21 20:03 dekimsey

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.

gillg avatar Jan 09 '22 13:01 gillg

My bad, I missunderstood some changes. Most of them are very useful. What about I'm not fan is officialize usage of native AWS SDK vars. Anyone else has an opinion ?

gillg avatar Jan 09 '22 22:01 gillg

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

github-actions[bot] avatar Jan 20 '23 19:01 github-actions[bot]

Build failed.

:heavy_check_mark: ansible-galaxy-importer SUCCESS in 4m 17s :heavy_check_mark: build-ansible-collection SUCCESS in 5m 53s :x: ansible-test-sanity-docker-devel FAILURE in 9m 59s (non-voting) :x: ansible-test-sanity-docker-milestone FAILURE in 8m 39s (non-voting) :x: ansible-test-sanity-docker-stable-2.12 FAILURE in 8m 52s :x: ansible-test-sanity-docker-stable-2.13 FAILURE in 8m 44s :x: ansible-test-sanity-docker-stable-2.14 FAILURE in 9m 54s :heavy_check_mark: ansible-test-units-amazon-aws-python36 SUCCESS in 5m 58s :heavy_check_mark: ansible-test-units-amazon-aws-python38 SUCCESS in 5m 50s :heavy_check_mark: ansible-test-units-amazon-aws-python39 SUCCESS in 5m 50s :heavy_check_mark: ansible-test-units-amazon-aws-python310 SUCCESS in 5m 43s :heavy_check_mark: ansible-test-changelog SUCCESS in 2m 32s :heavy_check_mark: ansible-test-splitter SUCCESS in 2m 57s :x: integration-community.aws-1 FAILURE in 29m 57s :x: integration-community.aws-2 FAILURE in 28m 30s :x: integration-community.aws-3 FAILURE in 30m 13s :x: integration-community.aws-4 FAILURE in 31m 32s :x: integration-community.aws-5 FAILURE in 29m 09s :x: integration-community.aws-6 FAILURE in 28m 32s :heavy_check_mark: integration-community.aws-7 SUCCESS in 6m 02s :warning: integration-community.aws-8 SKIPPED :warning: integration-community.aws-9 SKIPPED :warning: integration-community.aws-10 SKIPPED :warning: integration-community.aws-11 SKIPPED :warning: integration-community.aws-12 SKIPPED :warning: integration-community.aws-13 SKIPPED :warning: integration-community.aws-14 SKIPPED :warning: integration-community.aws-15 SKIPPED :warning: integration-community.aws-16 SKIPPED :warning: integration-community.aws-17 SKIPPED :warning: integration-community.aws-18 SKIPPED :warning: integration-community.aws-19 SKIPPED :warning: integration-community.aws-20 SKIPPED :warning: integration-community.aws-21 SKIPPED :warning: integration-community.aws-22 SKIPPED

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 avatar Jan 31 '23 11:01 tremble

Build succeeded.

:heavy_check_mark: ansible-galaxy-importer SUCCESS in 3m 55s :heavy_check_mark: build-ansible-collection SUCCESS in 5m 24s :heavy_check_mark: ansible-test-sanity-docker-devel SUCCESS in 8m 45s (non-voting) :heavy_check_mark: ansible-test-sanity-docker-milestone SUCCESS in 8m 57s (non-voting) :heavy_check_mark: ansible-test-sanity-docker-stable-2.12 SUCCESS in 8m 58s :heavy_check_mark: ansible-test-sanity-docker-stable-2.13 SUCCESS in 9m 51s :heavy_check_mark: ansible-test-sanity-docker-stable-2.14 SUCCESS in 9m 32s :heavy_check_mark: ansible-test-units-amazon-aws-python36 SUCCESS in 7m 04s :heavy_check_mark: ansible-test-units-amazon-aws-python38 SUCCESS in 5m 47s :heavy_check_mark: ansible-test-units-amazon-aws-python39 SUCCESS in 5m 35s :heavy_check_mark: ansible-test-units-amazon-aws-python310 SUCCESS in 6m 03s :heavy_check_mark: ansible-test-changelog SUCCESS in 2m 14s :heavy_check_mark: ansible-test-splitter SUCCESS in 2m 44s :heavy_check_mark: integration-community.aws-1 SUCCESS in 9m 19s :heavy_check_mark: integration-community.aws-2 SUCCESS in 8m 05s :heavy_check_mark: integration-community.aws-3 SUCCESS in 9m 02s :heavy_check_mark: integration-community.aws-4 SUCCESS in 9m 01s :heavy_check_mark: integration-community.aws-5 SUCCESS in 10m 00s :heavy_check_mark: integration-community.aws-6 SUCCESS in 10m 05s :heavy_check_mark: integration-community.aws-7 SUCCESS in 9m 13s :heavy_check_mark: integration-community.aws-8 SUCCESS in 9m 40s :warning: integration-community.aws-9 SKIPPED :warning: integration-community.aws-10 SKIPPED :warning: integration-community.aws-11 SKIPPED :warning: integration-community.aws-12 SKIPPED :warning: integration-community.aws-13 SKIPPED :warning: integration-community.aws-14 SKIPPED :warning: integration-community.aws-15 SKIPPED :warning: integration-community.aws-16 SKIPPED :warning: integration-community.aws-17 SKIPPED :warning: integration-community.aws-18 SKIPPED :warning: integration-community.aws-19 SKIPPED :warning: integration-community.aws-20 SKIPPED :warning: integration-community.aws-21 SKIPPED :warning: integration-community.aws-22 SKIPPED

Build succeeded (gate pipeline).

:heavy_check_mark: ansible-galaxy-importer SUCCESS in 5m 21s :heavy_check_mark: build-ansible-collection SUCCESS in 5m 35s :heavy_check_mark: ansible-test-sanity-docker-devel SUCCESS in 10m 12s (non-voting) :heavy_check_mark: ansible-test-sanity-docker-milestone SUCCESS in 12m 43s (non-voting) :heavy_check_mark: ansible-test-sanity-docker-stable-2.12 SUCCESS in 12m 35s :heavy_check_mark: ansible-test-sanity-docker-stable-2.13 SUCCESS in 12m 31s :heavy_check_mark: ansible-test-sanity-docker-stable-2.14 SUCCESS in 11m 47s :heavy_check_mark: ansible-test-units-amazon-aws-python36 SUCCESS in 6m 31s :heavy_check_mark: ansible-test-units-amazon-aws-python38 SUCCESS in 5m 52s :heavy_check_mark: ansible-test-units-amazon-aws-python39 SUCCESS in 6m 04s :heavy_check_mark: ansible-test-units-amazon-aws-python310 SUCCESS in 7m 06s :heavy_check_mark: ansible-test-changelog SUCCESS in 2m 20s :heavy_check_mark: ansible-test-splitter SUCCESS in 2m 31s :heavy_check_mark: integration-community.aws-1 SUCCESS in 10m 35s :heavy_check_mark: integration-community.aws-2 SUCCESS in 7m 29s :heavy_check_mark: integration-community.aws-3 SUCCESS in 9m 23s :heavy_check_mark: integration-community.aws-4 SUCCESS in 8m 46s :heavy_check_mark: integration-community.aws-5 SUCCESS in 9m 03s :heavy_check_mark: integration-community.aws-6 SUCCESS in 8m 25s :heavy_check_mark: integration-community.aws-7 SUCCESS in 7m 52s :heavy_check_mark: integration-community.aws-8 SUCCESS in 9m 13s :warning: integration-community.aws-9 SKIPPED :warning: integration-community.aws-10 SKIPPED :warning: integration-community.aws-11 SKIPPED :warning: integration-community.aws-12 SKIPPED :warning: integration-community.aws-13 SKIPPED :warning: integration-community.aws-14 SKIPPED :warning: integration-community.aws-15 SKIPPED :warning: integration-community.aws-16 SKIPPED :warning: integration-community.aws-17 SKIPPED :warning: integration-community.aws-18 SKIPPED :warning: integration-community.aws-19 SKIPPED :warning: integration-community.aws-20 SKIPPED :warning: integration-community.aws-21 SKIPPED :warning: integration-community.aws-22 SKIPPED

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/94d12952f12188940f759454032e870890191086/pr-514

Backported as https://github.com/ansible-collections/community.aws/pull/1683

🤖 @patchback I'm built with octomachinery and my source is open — https://github.com/sanitizers/patchback-github-app.

patchback[bot] avatar Jan 31 '23 12:01 patchback[bot]