ec2_vpc_nat_gateway show fails if EIP doesn't exist #1604

taehopark32 · 2023-06-06T21:49:47Z

SUMMARY

Fixes #1295

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

plugins/modules/ec2_vpc_nat_gateway

ADDITIONAL INFORMATION

softwarefactory-project-zuul · 2023-06-06T22:25:28Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/8598e269ee384b9aa5a3de5e3bb59992

✔️ ansible-galaxy-importer SUCCESS in 5m 02s
✔️ build-ansible-collection SUCCESS in 12m 45s
✔️ ansible-test-splitter SUCCESS in 4m 39s
✔️ integration-amazon.aws-1 SUCCESS in 19m 07s
Skipped 43 jobs

softwarefactory-project-zuul · 2023-06-15T15:13:00Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/1f7703703ef747be92c385cae71e4854

✔️ ansible-galaxy-importer SUCCESS in 4m 12s
✔️ build-ansible-collection SUCCESS in 12m 32s
✔️ ansible-test-splitter SUCCESS in 5m 10s
✔️ integration-amazon.aws-1 SUCCESS in 22m 15s
Skipped 43 jobs

github-actions · 2023-06-15T16:15:12Z

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

jillr

The new parameter needs to be added in a couple other places.
In the DOCUMENTATION block at the top of the file (make sure to document that the default value is False) https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#documentation-block

In the module argument_spec in main().
This is also where you set the default value as False. (The types in the documentation must match the types in the arg spec, linting rules will check this)
https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec

The value of the parameter must then be assigned to a variable in this section: https://github.com/ansible-collections/amazon.aws/blob/main/plugins/modules/ec2_vpc_nat_gateway.py#L881-L892
That value would be passed to functions that need it, instead of being defined through the code.

softwarefactory-project-zuul · 2023-06-15T21:21:01Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/3f03290b4cd348faac1ffc642034b288

✔️ ansible-galaxy-importer SUCCESS in 4m 57s
✔️ build-ansible-collection SUCCESS in 14m 14s
✔️ ansible-test-splitter SUCCESS in 4m 43s
✔️ integration-amazon.aws-1 SUCCESS in 22m 21s
Skipped 43 jobs

plugins/modules/ec2_vpc_nat_gateway.py

alinabuzachis · 2023-06-16T09:48:56Z

plugins/modules/ec2_vpc_nat_gateway.py

@@ -75,6 +75,11 @@
        When specifying this option, ensure you specify the eip_address parameter
        as well otherwise any subsequent runs will fail.
    type: str
+  default_create:
+    description:
+      - if EIP address is not found, create NAT gateway with EIP address as null.


I guess the description of this parameter must be carified. What happens when the users specifies an invalid allocation_id? Does default_create: True allow to still create the NAT gateway and allocate a new EIP?

Something like this:

When I(default_create=True), if I(eip_address) is set and it is invalid or does not exist, the NAT gateway is successfully created and a new EIP is automatically allocated.

When I(default_create=True), if I(allocation_id) is set and it is invalid or not found, the NAT gateway is successfully created and a new EIP is automatically allocated. (If this is true!!!)

When I(default_create=False), what happens?

Please feel free to rephrase and correct my suggestions.

changelogs/fragments/1604-c2_vpc_nat_gateway-fails-silently.yml

Co-authored-by: Bikouo Aubin <79859644+abikouo@users.noreply.github.com>

Co-authored-by: Alina Buzachis <abuzachis@redhat.com>

softwarefactory-project-zuul · 2023-06-16T14:26:31Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/bcbbf87a1fd34612b144305b67386ab8

✔️ ansible-galaxy-importer SUCCESS in 4m 13s
✔️ build-ansible-collection SUCCESS in 12m 54s
✔️ ansible-test-splitter SUCCESS in 4m 44s
✔️ integration-amazon.aws-1 SUCCESS in 22m 06s
Skipped 43 jobs

softwarefactory-project-zuul · 2023-06-16T16:45:24Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/7457322da94c4d009ad5fc0c39e6c5d3

✔️ ansible-galaxy-importer SUCCESS in 4m 43s
✔️ build-ansible-collection SUCCESS in 13m 21s
✔️ ansible-test-splitter SUCCESS in 4m 40s
✔️ integration-amazon.aws-1 SUCCESS in 19m 21s
Skipped 43 jobs

gravesm · 2023-06-16T17:50:56Z

plugins/modules/ec2_vpc_nat_gateway.py

+      - When I(default_create=True), if I(eip_address) is set and it is not found, the NAT
+        gateway is successfully created and a new EIP is automatically allocated.
+      - The validity of the I(allocation_id) is independent of the I(default_create). If
+        I(allocation_id) is set and it is invalid or not found, this will fail, regardless
+        of I(default_create).
+      - The validity of the I(eip_address) is independent of the I(default_create). If
+        I(eip_address) is set and it is invalid, this will fail, regardless of
+        I(default_create).
+      - When I (default_create=False), if I(eip_address) is set and it is invalid or does not
+        exist, this will fail.


This parameter only comes into play when eip_address has been set. IMO I would keep the allocation_id out of the description and state that the parameter has no effect when eip_address has not been set:

- When I(default_create=True) and I(eip_address) has been set, but not yet allocated, the NAT gateway is created and a new EIP is automatically allocated. - When I(default_create=False) and I(eip_address) has been set, but not yet allocated, the module will fail. - If I(eip_address) has not been set, this parameter has no effect.

That said, what you have looks correct and makes sense, so I don't have a problem with merging as is. I'll let @alinabuzachis make the call.

If allocation_id is out, it does not make sense to mention it. Agreed with @gravesm! @taehopark32 Thanks for updating docs!

alinabuzachis

@taehopark32 This looks great to me if you remove the reference to the allocation_id since the parameter applies only to eip_address.

alinabuzachis · 2023-06-16T18:11:22Z

plugins/modules/ec2_vpc_nat_gateway.py

+      - When I(default_create=True), if I(eip_address) is set and it is not found, the NAT
+        gateway is successfully created and a new EIP is automatically allocated.
+      - The validity of the I(allocation_id) is independent of the I(default_create). If
+        I(allocation_id) is set and it is invalid or not found, this will fail, regardless
+        of I(default_create).
+      - The validity of the I(eip_address) is independent of the I(default_create). If
+        I(eip_address) is set and it is invalid, this will fail, regardless of
+        I(default_create).
+      - When I (default_create=False), if I(eip_address) is set and it is invalid or does not
+        exist, this will fail.


If allocation_id is out, it does not make sense to mention it. Agreed with @gravesm! @taehopark32 Thanks for updating docs!

softwarefactory-project-zuul · 2023-06-16T18:56:34Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/ceb145c7ad554f97862c60c529a9c7df

✔️ ansible-galaxy-importer SUCCESS in 4m 55s
✔️ build-ansible-collection SUCCESS in 13m 03s
✔️ ansible-test-splitter SUCCESS in 7m 19s
✔️ integration-amazon.aws-1 SUCCESS in 20m 46s
Skipped 43 jobs

taehopark32 · 2023-06-16T19:19:53Z

@alinabuzachis Thank you. I fixed the description and passed all the tests.

We have already 2 approvals.

alinabuzachis · 2023-07-04T11:31:33Z

regate

softwarefactory-project-zuul · 2023-07-04T12:17:03Z

Build succeeded (gate pipeline).
https://ansible.softwarefactory-project.io/zuul/buildset/35ba235555354f96a9de7c3b330d417c

✔️ ansible-galaxy-importer SUCCESS in 4m 30s
✔️ build-ansible-collection SUCCESS in 13m 29s
✔️ ansible-test-splitter SUCCESS in 4m 47s
✔️ integration-amazon.aws-1 SUCCESS in 30m 21s
Skipped 43 jobs

patchback · 2023-07-04T12:17:08Z

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/6f207ec1b77bcee134df9e51d59215e8e6a948c7/pr-1604

Backported as #1642

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

ec2_vpc_nat_gateway show fails if EIP doesn't exist SUMMARY Fixes #1295 ISSUE TYPE Bugfix Pull Request COMPONENT NAME plugins/modules/ec2_vpc_nat_gateway ADDITIONAL INFORMATION Reviewed-by: Jill R Reviewed-by: Bikouo Aubin Reviewed-by: Alina Buzachis Reviewed-by: Mike Graves <mgraves@redhat.com> (cherry picked from commit 6f207ec)

[PR #1604/6f207ec1 backport][stable-6] ec2_vpc_nat_gateway show fails if EIP doesn't exist This is a backport of PR #1604 as merged into main (6f207ec). SUMMARY Fixes #1295 ISSUE TYPE Bugfix Pull Request COMPONENT NAME plugins/modules/ec2_vpc_nat_gateway ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis

changes to the module

16fc659

taehopark32 changed the title ~~ec2_vpc_nat_gateway show fails if EIP doesn't exist~~ WIP: ec2_vpc_nat_gateway show fails if EIP doesn't exist Jun 6, 2023

taehopark32 changed the title ~~WIP: ec2_vpc_nat_gateway show fails if EIP doesn't exist~~ [WIP] ec2_vpc_nat_gateway show fails if EIP doesn't exist Jun 7, 2023

taehopark32 added 2 commits June 15, 2023 10:20

add test cases

8e1ad94

added fragment for 1604

ca18295

taehopark32 changed the title ~~[WIP] ec2_vpc_nat_gateway show fails if EIP doesn't exist~~ ec2_vpc_nat_gateway show fails if EIP doesn't exist Jun 15, 2023

jillr previously requested changes Jun 15, 2023

View reviewed changes

taehopark32 added 2 commits June 15, 2023 16:01

changes to the test cases and add documentation

9f28322

fixed documentation

3c82a8d

abikouo previously requested changes Jun 16, 2023

View reviewed changes

plugins/modules/ec2_vpc_nat_gateway.py Show resolved Hide resolved

alinabuzachis reviewed Jun 16, 2023

View reviewed changes

changelogs/fragments/1604-c2_vpc_nat_gateway-fails-silently.yml Outdated Show resolved Hide resolved

taehopark32 and others added 2 commits June 16, 2023 09:48

Update plugins/modules/ec2_vpc_nat_gateway.py

e1b6472

Co-authored-by: Bikouo Aubin <79859644+abikouo@users.noreply.github.com>

Update changelogs/fragments/1604-c2_vpc_nat_gateway-fails-silently.yml

5ac39a6

Co-authored-by: Alina Buzachis <abuzachis@redhat.com>

more specific description for paramter default_create

a5dfb7b

gravesm reviewed Jun 16, 2023

View reviewed changes

alinabuzachis reviewed Jun 16, 2023

View reviewed changes

alinabuzachis added the backport-6 PR should be backported to the stable-6 branch label Jun 16, 2023

update the description for the default_create parameter

0b93d22

gravesm approved these changes Jun 21, 2023

View reviewed changes

alinabuzachis approved these changes Jun 27, 2023

View reviewed changes

alinabuzachis requested a review from abikouo June 27, 2023 09:19

alinabuzachis requested a review from jillr June 27, 2023 09:20

alinabuzachis added the mergeit Merge the PR (SoftwareFactory) label Jul 3, 2023

softwarefactory-project-zuul bot merged commit 6f207ec into ansible-collections:main Jul 4, 2023

patchback bot mentioned this pull request Jul 4, 2023

[PR #1604/6f207ec1 backport][stable-6] ec2_vpc_nat_gateway show fails if EIP doesn't exist #1642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ec2_vpc_nat_gateway show fails if EIP doesn't exist #1604

ec2_vpc_nat_gateway show fails if EIP doesn't exist #1604

taehopark32 commented Jun 6, 2023 •

edited

Loading

softwarefactory-project-zuul bot commented Jun 6, 2023

softwarefactory-project-zuul bot commented Jun 15, 2023

github-actions bot commented Jun 15, 2023 •

edited

Loading

jillr left a comment

softwarefactory-project-zuul bot commented Jun 15, 2023

alinabuzachis Jun 16, 2023 •

edited

Loading

softwarefactory-project-zuul bot commented Jun 16, 2023

softwarefactory-project-zuul bot commented Jun 16, 2023

gravesm Jun 16, 2023

alinabuzachis Jun 16, 2023

alinabuzachis left a comment

alinabuzachis Jun 16, 2023

softwarefactory-project-zuul bot commented Jun 16, 2023

taehopark32 commented Jun 16, 2023

alinabuzachis commented Jul 4, 2023

softwarefactory-project-zuul bot commented Jul 4, 2023

patchback bot commented Jul 4, 2023 •

edited

Loading

ec2_vpc_nat_gateway show fails if EIP doesn't exist #1604

ec2_vpc_nat_gateway show fails if EIP doesn't exist #1604

Conversation

taehopark32 commented Jun 6, 2023 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

softwarefactory-project-zuul bot commented Jun 6, 2023

softwarefactory-project-zuul bot commented Jun 15, 2023

github-actions bot commented Jun 15, 2023 • edited Loading

Docs Build 📝

jillr left a comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jun 15, 2023

alinabuzachis Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jun 16, 2023

softwarefactory-project-zuul bot commented Jun 16, 2023

gravesm Jun 16, 2023

Choose a reason for hiding this comment

alinabuzachis Jun 16, 2023

Choose a reason for hiding this comment

alinabuzachis left a comment

Choose a reason for hiding this comment

alinabuzachis Jun 16, 2023

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jun 16, 2023

taehopark32 commented Jun 16, 2023

alinabuzachis commented Jul 4, 2023

softwarefactory-project-zuul bot commented Jul 4, 2023

patchback bot commented Jul 4, 2023 • edited Loading

Backport to stable-6: 💚 backport PR created

taehopark32 commented Jun 6, 2023 •

edited

Loading

github-actions bot commented Jun 15, 2023 •

edited

Loading

alinabuzachis Jun 16, 2023 •

edited

Loading

patchback bot commented Jul 4, 2023 •

edited

Loading