-
Notifications
You must be signed in to change notification settings - Fork 398
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
Bugfix/ec2 instance mod sgs #22
Bugfix/ec2 instance mod sgs #22
Conversation
Additionally switch from *_facts to *_info
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.
Thanks for the PR including tests.
I think some of the logic can be simplified.
Co-authored-by: Mark Chappell <[email protected]>
Co-authored-by: Mark Chappell <[email protected]>
Instead of looping over the param_mapping for security groups they are called out specifically. Refs #21
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.
Looks good
plugins/modules/ec2_instance.py
Outdated
default_vpc = get_default_vpc(ec2) | ||
if default_vpc is None: | ||
module.fail_json( | ||
msg="No default subnet could be found - you must include a VPC subnet ID (vpc_subnet_id parameter) to create an instance") |
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.
This error should probably be about modifying security groups instead of creating an instance
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.
What do you think about the new verbiage?
When a subnet is not defined and cannot be found generate an error message that reflects operating on a security group. Refs #21
I've added a changelog fragment but I think we're good to merge this now. |
Fixes #54174 * Added SG handling for existing instances + some cleanup * tests(ec2_instance): Tests for SG modifications to existing instances * tests(ec2_instance): Test simultaneous state and SG changes * refactor(ec2_instance): Move security out of for loop * style(ec2_instance): Update fail message to reflect security groups * Add changelog Co-authored-by: Andrea Tartaglia <[email protected]> Co-authored-by: Mark Chappell <[email protected]>
Fixes #54174 * Added SG handling for existing instances + some cleanup * tests(ec2_instance): Tests for SG modifications to existing instances * tests(ec2_instance): Test simultaneous state and SG changes * refactor(ec2_instance): Move security out of for loop * style(ec2_instance): Update fail message to reflect security groups * Add changelog Co-authored-by: Andrea Tartaglia <[email protected]> Co-authored-by: Mark Chappell <[email protected]>
…tions#22) * Change github url in galaxy.yml from git@ to https://
Fixes #54174 * Added SG handling for existing instances + some cleanup * tests(ec2_instance): Tests for SG modifications to existing instances * tests(ec2_instance): Test simultaneous state and SG changes * refactor(ec2_instance): Move security out of for loop * style(ec2_instance): Update fail message to reflect security groups * Add changelog Co-authored-by: Andrea Tartaglia <[email protected]> Co-authored-by: Mark Chappell <[email protected]>
SUMMARY
This fixes the issue below, where security groups were not changed if the instance already existed.
Fixes #21
ISSUE TYPE
COMPONENT NAME
ec2_instance.py
ADDITIONAL INFORMATION
See ansible/ansible#67778 and ansible/ansible#55712 for previous discussions.