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

Return all infos of a VPC peering connection in ec2_vpc_peer module #355

Merged
merged 12 commits into from
Apr 9, 2021
Merged

Return all infos of a VPC peering connection in ec2_vpc_peer module #355

merged 12 commits into from
Apr 9, 2021

Conversation

stefanhorning
Copy link
Contributor

@stefanhorning stefanhorning commented Jan 12, 2021 •
edited
Loading

SUMMARY

While boto3 returns a whole lot of useful information on a VPC peering connection, this module threw those away and only returned the ID. This PR changes that to pass through all information, which is also quite valuable when updating routing tables etc.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_vpc_peer module

ADDITIONAL INFORMATION
  • Add propper testing
  • Add better docs?

@ansibullbot
Copy link

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

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage plugins plugin (any type) labels Jan 13, 2021
@gravesm
Copy link
Member

gravesm commented Mar 19, 2021

@stefanhorning Are you still working on this? Since we can't integration test this, it would be helpful to see task output before and after. We would also like to see module docs. If you are willing to, it would be nice to have unit tests for this module.

@ansibullbot ansibullbot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Mar 19, 2021
@stefanhorning
Copy link
Contributor Author

Sorry, didn't have time for Ansible stuff for a while, will try to get the TODOs out of the way, perhaps this or next week.

@tremble
Copy link
Contributor

tremble commented Mar 29, 2021

#501 should add some initial tests that could be built on.

@stefanhorning
Copy link
Contributor Author

Thanks @tremble that helps to speed things up!

@tremble tremble mentioned this pull request Apr 3, 2021
Merged
@ansibullbot ansibullbot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 9, 2021
@stefanhorning
Copy link
Contributor Author

Tests are actually green now, but one still marked as unstable, not sure what to make of that
https://app.shippable.com/github/ansible-collections/community.aws/runs/2249/23/tests

@tremble
Copy link
Contributor

tremble commented Apr 9, 2021

The failed / unstable test was a race condition (an _info straight after the making a change returned the previous status). We can ignore it for now.

@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 Apr 9, 2021
tremble
tremble requested changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The change itself looks good.

  1. Please add a changelog fragment
  2. Please add some integration tests. They don't need to be thorough on every call, but maybe test all values on the accept idempotency test and test result.vpc_peering_connection.vpc_peering_connection_id == peer_id_1 on the rest.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Apr 9, 2021
@stefanhorning
Copy link
Contributor Author

stefanhorning commented Apr 9, 2021 •
edited
Loading

@tremble yes, I am actually still on the above mentioned TODOs right now 😉 :

  • Add propper testing
  • Add better RETURN docs

Will also do

  • Add changelog fragment

@ansibullbot ansibullbot added integration tests/integration tests tests labels Apr 9, 2021
tremble
tremble requested changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Minor typo, but otherwise looking good.

tests/integration/targets/ec2_vpc_peer/tasks/main.yml Outdated Show resolved Hide resolved
@stefanhorning
Copy link
Contributor Author

Ready for final review now.

@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 Apr 9, 2021
@tremble tremble merged commit faa7d64 into ansible-collections:main Apr 9, 2021
@tremble
Copy link
Contributor

tremble commented Apr 9, 2021

Thanks for your work on this @stefanhorning

alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 16, 2021
@stefanhorning
[promoted]Return all infos of a VPC peering connection in ec2_vpc_pee…
be9736c 
…r module (ansible-collections#355)

* Return all infos of a VPC peering connection in ec2_vpc_peer module.
* More extensive tests for vpc_peer module. Also got rid of redundant helper method in vpc_peer module

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@faa7d64
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@stefanhorning
Return all infos of a VPC peering connection in ec2_vpc_peer module (a…
bdae881 
…nsible-collections#355)

* Return all infos of a VPC peering connection in ec2_vpc_peer module.
* More extensive tests for vpc_peer module. Also got rid of redundant helper method in vpc_peer module
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@stefanhorning
Return all infos of a VPC peering connection in ec2_vpc_peer module (a…
12850c6 
…nsible-collections#355)

* Return all infos of a VPC peering connection in ec2_vpc_peer module.
* More extensive tests for vpc_peer module. Also got rid of redundant helper method in vpc_peer module
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
@stefanhorning @danielcotton
Return all infos of a VPC peering connection in ec2_vpc_peer module (a…
ea8ee49 
…nsible-collections#355)

* Return all infos of a VPC peering connection in ec2_vpc_peer module.
* More extensive tests for vpc_peer module. Also got rid of redundant helper method in vpc_peer module
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
@tremble
Cloudformation: use is_boto3_error_message (ansible-collections#355)
0164b48 
Cloudformation: use is_boto3_error_message

Reviewed-by: https://github.com/apps/ansible-zuul
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tremble tremble tremble approved these changes

Assignees
No one assigned
Labels
community_review feature This issue/PR relates to a feature request integration tests/integration module module plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stefanhorning @ansibullbot @gravesm @tremble