ec2_vpc_nat_gateway cleanup #445

alinabuzachis · 2021-02-25T14:12:42Z

SUMMARY

Cleanup ec2_vpc_nat_gateway

ISSUE TYPE

COMPONENT NAME

ec2_vpc_nat_gateway

ansibullbot · 2021-02-25T14:20:01Z

cc @Etherdaemon @jillr @jonhadfield @linuxdynasty @s-hertel @tremble @wimnat
click here for bot help

tremble · 2021-02-25T14:36:26Z

Test failures look to be due to a lack of retry decorators on ec2_vpc_nat_gateway_info

alinabuzachis · 2021-02-25T15:02:59Z

@tremble yes, thank you. Should I add the decorator in this PR or open a new one? This PR is for the overall cleanup of the ec2_vpc_nat_gateway module and I think it would be better to merge #436 before. I can add the decorator in #436 if you prefer.

tremble · 2021-02-25T15:37:56Z

In the strictest sense it can be its own PR, which I'd happily review and merge pretty quickly.

In the past I've bundled the and _info cleanup together in a couple of cases, because the tests tend to be linked (and getting reviews was difficult before yourself and the other new cloud team members started).

alinabuzachis · 2021-02-25T16:21:50Z

@tremble Ok, I will do it in a while and I will ping you then. Thank you.

jillr

Good progress here

jillr · 2021-03-01T20:12:00Z

plugins/modules/ec2_vpc_nat_gateway.py

        return True, ''

    ip_released = False
    try:
        client.describe_addresses(aws_retry=True, AllocationIds=[allocation_id])
-    except botocore.exceptions.ClientError as e:
+    except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:


There might be a more specific failure that we could catch here with is_boto3_error_code (probably InvalidAllocationID.NotFound, not sure about any others) to do the return True, str(e), and handle general exceptions separately.

@jillr Thank you for reviewing. I tried to add the catch on this error. Let me know if it makes sense to you.

@jillr I noticed adding the explicit catch of the InvalidAllocationID.NotFound error implies removing the botocore.exceptions.ClientError because InvalidAllocationID.NotFound is already catched by ClientError. Otherwise, I can catch only InvalidAllocationID.NotFound and then consider the except botocore.exceptions.BotoCoreError (this is what I've actually done right now). Let me know if this works for you.

It should be possible to catch both BotoCoreError and ClientError.

While pylint thinks that except is_boto3_error_code('InvalidAllocationID.NotFound') catches all ClientError, this is actually incorrect and due to a bug in pylint. pylint-dev/pylint#2174

@tremble Ok, my Shippable check was failing due to this duplicate catch. I added again the the botocore.exceptions.ClientError. Thanks.

tremble · 2021-03-09T15:25:37Z

plugins/modules/ec2_vpc_nat_gateway.py

        # IP address likely already released
        # Happens with gateway in 'deleted' state that
        # still lists associations
-        return True, str(e)
+        return True, e
+    except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:


Suggested change

except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:

except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: # pylint: disable=duplicate-except

Thanks. I forget to commit it. Could you kindly have a look as well at this #436 (when you will have some time)?

ansibullbot · 2021-03-10T11:54:29Z

@alinabuzachis this PR contains the following merge commits:

eb356f0

Please rebase your branch to remove these commits.

click here for bot help

* use custm waiters to manage NAT gateway states (deleted and available) * imporve error handling * improve documentation examples * code cleaning Signed-off-by: Alina Buzachis <[email protected]>

jillr

Couple small things here, thanks @alinabuzachis for all your work on this

jillr · 2021-03-15T19:11:23Z

plugins/modules/ec2_vpc_nat_gateway.py

                "EIP {0} does not exist".format(eip_address)
            )
+    except is_boto3_error_code('InvalidAddress.Malformed') as e:
+        allocation_id = None


If the EIP the user provides is malformed we should probably return that information to the user so that they can troubleshoot their playbook, we're returning an empty msg now. For a malformed EIP we should probably fail here rather than returning back to pre_create since we can't proceed with the information provided in the playbook.

I added fail_json for this case. Let me know if this works for you. Thank you @jillr.

jillr · 2021-03-15T19:13:18Z

plugins/modules/ec2_vpc_nat_gateway.py

+    except is_boto3_error_code('InvalidAddress.Malformed') as e:
+        allocation_id = None
+    except is_boto3_error_code('InvalidAddress.NotFound') as e:  # pylint: disable=duplicate-except
+        allocation_id = None


We probably also want to exit here as well and return the error to the user.

Thank you for reviewing @jillr. I added the specific msg for this case. As allocation_id will be None, it will exit propagating the message. https://github.com/ansible-collections/community.aws/pull/445/files#diff-847118175f7fbd015daf612a3319157ca22874f706c01f26348f4f86f67384d9R742-R749 Is this working for you?

Yep, this makes sense to me. Thanks!

Signed-off-by: Alina Buzachis <[email protected]>

jillr

lgtm

* ec2_vpc_nat_gateway overall cleanup * use custm waiters to manage NAT gateway states (deleted and available) * imporve error handling * improve documentation examples * code cleaning Signed-off-by: Alina Buzachis <[email protected]>

* ec2_vpc_nat_gateway overall cleanup * use custm waiters to manage NAT gateway states (deleted and available) * imporve error handling * improve documentation examples * code cleaning Signed-off-by: Alina Buzachis <[email protected]> This commit was initially merged in https://github.com/ansible-collections/community.aws See: ansible-collections@9b60a2d

* ec2_vpc_nat_gateway overall cleanup * use custm waiters to manage NAT gateway states (deleted and available) * imporve error handling * improve documentation examples * code cleaning Signed-off-by: Alina Buzachis <[email protected]>

* Add waiter for ec2_vpc_nat_gateway (ansible-collections#445) * Added NatGatewayAvailable waiter Signed-off-by: Alina Buzachis <[email protected]>

* ec2_vpc_nat_gateway overall cleanup * use custm waiters to manage NAT gateway states (deleted and available) * imporve error handling * improve documentation examples * code cleaning Signed-off-by: Alina Buzachis <[email protected]> This commit was initially merged in https://github.com/ansible-collections/community.aws See: ansible-collections@9b60a2d

ansibullbot added WIP Work in progress module module needs_triage plugins plugin (any type) small_patch Hopefully easy to review labels Feb 25, 2021

ansibullbot removed the needs_triage label Feb 25, 2021

alinabuzachis force-pushed the cleanup_ec2_vpc_nat_gateway branch from 8126d8b to 3741139 Compare February 25, 2021 16:18

ansibullbot added integration tests/integration tests tests and removed small_patch Hopefully easy to review labels Feb 25, 2021

jillr reviewed Mar 1, 2021

View reviewed changes

alinabuzachis force-pushed the cleanup_ec2_vpc_nat_gateway branch 2 times, most recently from ebf852c to 2c00a76 Compare March 4, 2021 18:02

alinabuzachis mentioned this pull request Mar 4, 2021

Add the waiter for ec2_vpc_nat_gateway ansible-collections/amazon.aws#279

Merged

alinabuzachis force-pushed the cleanup_ec2_vpc_nat_gateway branch 6 times, most recently from 0eba859 to 79ce47f Compare March 9, 2021 14:43

tremble reviewed Mar 9, 2021

View reviewed changes

alinabuzachis force-pushed the cleanup_ec2_vpc_nat_gateway branch 2 times, most recently from 6dc30db to f98c541 Compare March 9, 2021 22:02

ansibullbot added the merge_commit This PR contains at least one merge commit. Please resolve! label Mar 10, 2021

ansibullbot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Mar 10, 2021

alinabuzachis force-pushed the cleanup_ec2_vpc_nat_gateway branch 2 times, most recently from edec340 to 923f544 Compare March 10, 2021 17:02

ansibullbot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Mar 10, 2021

alinabuzachis changed the title ~~[WIP] ec2_vpc_nat_gateway cleanup~~ ec2_vpc_nat_gateway cleanup Mar 11, 2021

ansibullbot added community_review and removed WIP Work in progress labels Mar 11, 2021

alinabuzachis force-pushed the cleanup_ec2_vpc_nat_gateway branch 3 times, most recently from 2e1a7e6 to 38f3b66 Compare March 11, 2021 18:55

ec2_vpc_nat_gateway overall cleanup

4c0b187

* use custm waiters to manage NAT gateway states (deleted and available) * imporve error handling * improve documentation examples * code cleaning Signed-off-by: Alina Buzachis <[email protected]>

jillr reviewed Mar 15, 2021

View reviewed changes

* Address reviewer's comments

c555d66

Signed-off-by: Alina Buzachis <[email protected]>

alinabuzachis force-pushed the cleanup_ec2_vpc_nat_gateway branch from 38f3b66 to c555d66 Compare March 16, 2021 13:46

jillr approved these changes Mar 16, 2021

View reviewed changes

jillr merged commit 9b60a2d into ansible-collections:main Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ec2_vpc_nat_gateway cleanup #445

ec2_vpc_nat_gateway cleanup #445

alinabuzachis commented Feb 25, 2021

ansibullbot commented Feb 25, 2021

tremble commented Feb 25, 2021

alinabuzachis commented Feb 25, 2021

tremble commented Feb 25, 2021

alinabuzachis commented Feb 25, 2021

jillr left a comment

jillr Mar 1, 2021

alinabuzachis Mar 2, 2021

alinabuzachis Mar 9, 2021

tremble Mar 9, 2021

alinabuzachis Mar 9, 2021

tremble Mar 9, 2021

alinabuzachis Mar 9, 2021

ansibullbot commented Mar 10, 2021

jillr left a comment

jillr Mar 15, 2021

alinabuzachis Mar 16, 2021

jillr Mar 15, 2021

alinabuzachis Mar 16, 2021 •

edited

Loading

jillr Mar 16, 2021

jillr left a comment

	except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
	except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: # pylint: disable=duplicate-except

ec2_vpc_nat_gateway cleanup #445

ec2_vpc_nat_gateway cleanup #445

Conversation

alinabuzachis commented Feb 25, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Feb 25, 2021

tremble commented Feb 25, 2021

alinabuzachis commented Feb 25, 2021

tremble commented Feb 25, 2021

alinabuzachis commented Feb 25, 2021

jillr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibullbot commented Mar 10, 2021

jillr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinabuzachis Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jillr left a comment

Choose a reason for hiding this comment

alinabuzachis Mar 16, 2021 •

edited

Loading