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_instance test cleanup #63708

Merged
merged 14 commits into from
Oct 21, 2019
Merged

ec2_instance test cleanup #63708

merged 14 commits into from
Oct 21, 2019

Conversation

tremble
Copy link
Contributor

@tremble tremble commented Oct 19, 2019

SUMMARY

Attempts to stabilise the ec2_instance tests (see also #58650)

  • Cleanup docs
  • Migrate tests to use module_defaults
  • Make sure we clean up resources when we're finished with them
  • Pause after creating roles to ensure creation actually completed

CC: @s-hertel

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/modules/cloud/amazon/ec2_instance.py
test/sanity/ignore.txt

ADDITIONAL INFORMATION

Found one 'interesting' quirk - 'wait' only waits for the completion of actions initiated by the specific call.
If you pass a filter or multiple instance_ids and they're already transitioning to the new state, then 'wait' will return straight away rather than waiting for the instances to reach the desired state. This is problematic when dealing with mass-removals where you need to wait for dependencies to be 'gone' before deleting them (eg waiting on instance termination before deleting ENIs/volumes)

@ansibot
Copy link
Contributor

ansibot commented Oct 19, 2019

@ansibot
Copy link
Contributor

ansibot commented Oct 19, 2019

@tremble, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 aws bug This issue/PR relates to a bug. cloud module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Oct 19, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 20, 2019
@tremble
Copy link
Contributor Author

tremble commented Oct 20, 2019

I believe the code's good at this point, so reviews would be appreciated.

Over the course of the next couple of hours I'm going to try to trigger additional test runs in the Shippable environment by rebasing, cleaning up unused/duplicate files and tweaking the documentation a little bit. However, I believe I've tracked down the cause of the instability...

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Oct 20, 2019
@ansibot
Copy link
Contributor

ansibot commented Oct 20, 2019

@tremble This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Oct 20, 2019
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Oct 20, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 20, 2019
@tremble
Copy link
Contributor Author

tremble commented Oct 21, 2019

@s-hertel I'll push more rebases through out the day, but I think the tests are now consistently passing.

Copy link
Contributor

@s-hertel s-hertel 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 good. Thanks for cleaning up so much.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Oct 21, 2019
@ansibot
Copy link
Contributor

ansibot commented Oct 21, 2019

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/ec2_instance/aliases:0:0: conflicting alias `shippable/aws/group[1-2]` and `unsupported`

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Oct 21, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 21, 2019
@s-hertel s-hertel merged commit 145b79e into ansible:devel Oct 21, 2019
@s-hertel
Copy link
Contributor

Thanks @tremble!

@tremble tremble deleted the tests_ec2_instance branch October 21, 2019 13:05
@ansible ansible locked and limited conversation to collaborators Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 aws bug This issue/PR relates to a bug. cloud core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants