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.

Sign up for GitHub

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

Jump to bottom

remove instance-state-name inverse filters in favor of standard value… #237

Merged
merged 3 commits into from
Jan 22, 2021
Merged

remove instance-state-name inverse filters in favor of standard value… #237

merged 3 commits into from
Jan 22, 2021

Conversation

IamNobleSix
Copy link
Contributor

remove instance-state-name inverse filters in favor of standard value filters on other 4 accepted options to fix bad aws return

SUMMARY

This fixes: #235

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

amazon aws ec2.py

ADDITIONAL INFORMATION

Instead of using inverse filters (which to my knowledge are not allowed by any or all version of aws cli and can cause issues as noted in #235) use the other 4 allowed values as normal outlined in https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-instances.html for instance-state-name

remove instance-state-name inverse filters in favor of standard value…
80f3999 
… filters on other 4 accepted options to fix bad aws return
@IamNobleSix IamNobleSix mentioned this pull request Jan 14, 2021
Closed
@ansibullbot
Copy link

cc @jillr @lwade @s-hertel @tgerla @tremble @wimnat
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review has_issue module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Jan 14, 2021
gravesm
gravesm requested changes Jan 15, 2021
View reviewed changes
Copy link
Member

@gravesm gravesm left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, @IamNobleSix. This change looks good. We need a changelog fragment before we can merge: https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment.

It would also be a good idea to add a test for this, which would have caught this regression before it went out. In this case, I think just adding a task to stop the second instance here: https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/ec2/tasks/main.yml#L138 and then assert that it is in fact stopped should be sufficient.

@gravesm gravesm self-assigned this Jan 15, 2021
@IamNobleSix
Copy link
Contributor Author

Hi @gravesm that sounds good to me. I will get working on that today. Thanks.

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Jan 15, 2021
@ansibullbot ansibullbot added integration tests/integration tests tests and removed small_patch Hopefully easy to review labels Jan 18, 2021
@IamNobleSix IamNobleSix requested a review from gravesm January 18, 2021 19:08
@IamNobleSix
Copy link
Contributor Author

Stop test created using similar logic to already established terminate test on line 103.

gravesm
gravesm approved these changes Jan 19, 2021
View reviewed changes
Copy link
Member

@gravesm gravesm 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 your work on this, @IamNobleSix!

@gravesm gravesm requested review from goneri, jillr and tremble January 19, 2021 13:37
@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 19, 2021
@goneri
Copy link
Member

goneri commented Jan 22, 2021

Thank you @IamNobleSix for your contribution.

@goneri goneri merged commit 1c51d9a into ansible-collections:main Jan 22, 2021
@abikouo abikouo mentioned this pull request Mar 3, 2021
Closed
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
@jillr
aws_ssm: Namespace S3 buckets and delete transferred files (ansible-c…
921bd53 
…ollections#237)

Files transferred to instances via the SSM connection plugin should use
folders within the bucket that are namespaced per-host, to prevent collisions.
Files should also be deleted from buckets when they are no longer required.

Fixes: ansible-collections#221
Fixes: ansible-collections#222

Based on work by abeluck

changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jillr jillr jillr approved these changes

@gravesm gravesm gravesm approved these changes

@goneri goneri goneri approved these changes

@tremble tremble Awaiting requested review from tremble

Assignees

@gravesm gravesm

Labels
bug This issue/PR relates to a bug community_review has_issue integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AWS inverse filter in Ansible 2.10.5 amazon aws ec2.py causing bad return and empty array during stop task.
5 participants
@IamNobleSix @ansibullbot @goneri @gravesm @jillr