feat(module/vpc-cagw): Add Carrier Gateway modules #1353
Conversation
mtulio
force-pushed
the
feat-mod-vpc_cagw
branch
from
04a34a4 to
d1e5e95
Compare
ansibullbot
added
community_review
module
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
There was a problem hiding this comment.
Thanks for this work, it looks like a really good start. Most of my comments are stylistic niggles.
It looks like you've copied the framework from an existing module (which is fine) however you've pulled in some cruft which we probably want to avoid. We're slowly cleaning things up, so please understand that we might ask you to change things that you copied from other modules.
For net-new modules we generally require integration tests. These integration tests are just Ansible playbooks and can be found in tests/integration/targets/ the ec2_vpc_vgw tests are probably a good example to start with. Just copy them over to tests/integration/targets/<module_name> and update the aliases file to include the _info module.
I suspect it'll take a little while to get the tests to work in CI due to the need to enable a Wavelength AZ (which has other side effects), so I'd suggest adding unsupported to the aliases file, which means they can be run manually outside of the main CI, but we need to do some work to get them running in CI.
We have some more info about this in our dev guidelines:
https://github.com/ansible-collections/amazon.aws/blob/main/docs/docsite/rst/dev_guidelines.rst#integration-tests-for-aws-modules
|
Hi @tremble , Thanks for your quick review and feedback!
Yes, those plugins were based on the Internet Gateway plugins (amazon.aws.ec2_vpc_igw*), which is similar Carrier Gateways, but the module is also quite old. I am also quite new in ansible module development, thanks for your suggestions and feel free to request any change, even through it is to refact based on newer modules. :)
Perfect, I will work on it.
ACK. Let me write the tests and see if we can get more insights about it, but having a better handler for the exception when there are no Wavelength zone group enabled it seems to be something nice to have on the module. |
We have two helpers "is_boto3_error_code" and "is_boto3_error_message" which will probably assist here: Similarly: |
ansibullbot
added
needs_revision
|
recheck |
Co-authored-by: Markus Bergholz <[email protected]>
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 26s |
|
recheck |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 6m 05s |
There was a problem hiding this comment.
I've pushed some minor changes:
- We're starting to move towards the "Black" formatting style
- Some of the imports have been split up / moved around (we started having issues with circular dependencies)
- The documentation fragments got renamed to better describe what they do.
|
Build failed. ❌ ansible-galaxy-importer FAILURE in 3m 55s |
|
I'm sorry about the delay getting back to reviewing this, without the CGW stuff enabled in an account it's tough to properly test this, so I fear we'll have to trust that the modules work for now. |
tremble
force-pushed
the
feat-mod-vpc_cagw
branch
from
e4571c2 to
3616de5
Compare
|
Build failed. ❌ ansible-galaxy-importer FAILURE in 3m 46s |
|
regate |
|
regate |
|
regate |
|
Build succeeded (gate pipeline). ✔️ ansible-galaxy-importer SUCCESS in 4m 35s |
…926) ec2_vpc_route_table: Add support for Route entry for Carrier Gateway SUMMARY Add support for VPC Carrear Gateways entry on route table. ISSUE TYPE Feature Pull Request COMPONENT NAME module/ec2_vpc_route_table ADDITIONAL INFORMATION Support Carrier Gateway route on Route Table module using the same strategy of Nat GW, discovering the prefix of resource name cagw-. Not directly related, but testing in the same solution the cagw modules: ansible-collections/community.aws#1353 ec2_carrier_gateway ec2_carrier_gateway_info Reviewed-by: Mark Chappell Reviewed-by: Marco Braga
…ns#1353) feat(module/vpc-cagw): Add Carrier Gateway modules SUMMARY New modules to manage VPC Carrear Gateways. ISSUE TYPE New Module Pull Request COMPONENT NAME modules (new): ec2_carrier_gateway ec2_carrier_gateway_info ADDITIONAL INFORMATION $ ansible localhost -m ec2_vpc_cagw_info localhost | SUCCESS => { "carrier_gateways": [ { "carrier_gateway_id": "cagw-037df45cae5362d59", "tags": { "Name": "test1-54dsl-vpc-cagw" }, "vpc_id": "vpc-069cabb60c7e7fc6d" } ], "changed": false } $ ansible localhost -m ec2_carrier_gateway -a "state=absent vpc_id=vpc-069cabb60c7e7fc6d carrier_gateway_id=cagw-037df45cae5362d59" localhost | CHANGED => { "changed": true } $ ansible localhost -m ec2_carrier_gateway_info localhost | SUCCESS => { "carrier_gateways": [], "changed": false } $ ansible localhost -m ec2_carrier_gateway-a "vpc_id=vpc-069cabb60c7e7fc6d" localhost | CHANGED => { "carrier_gateway_id": "cagw-095f998ebdcb5ef86", "changed": true, "tags": {}, "vpc_id": "vpc-069cabb60c7e7fc6d" } $ ansible localhost -m ec2_carrier_gateway_info localhost | SUCCESS => { "carrier_gateways": [ { "carrier_gateway_id": "cagw-095f998ebdcb5ef86", "tags": {}, "vpc_id": "vpc-069cabb60c7e7fc6d" } ], "changed": false } Reviewed-by: Mark Chappell Reviewed-by: Marco Braga Reviewed-by: Markus Bergholz <[email protected]>
SUMMARY
New modules to manage VPC Carrear Gateways.
ISSUE TYPE
COMPONENT NAME
modules (new):
ADDITIONAL INFORMATION