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.

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

update: Skips deleted and in deletion NAT gateways from nuking, adds test for config file lookup #313

Merged

Conversation

GiamPy5
Copy link
Contributor

@GiamPy5 GiamPy5 commented Jun 8, 2022

Description

Solves #310.

Edit: I initially thought it would solve issue number 310, but the real problem is in the README.md since it's misleading and doesn't match with the Config key in the Go code (https://github.com/gruntwork-io/cloud-nuke/blob/master/config/config.go#L16). So this PR actually just adds a test and verifies that the code works properly (which it does), and adds a small verification step in the lookup function to make sure that it doesn't retrieve NAT gateways that are deleted or in deletion.

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

  • Added test to verify that looking up NAT Gateways with the configuration file works.
  • Updated the NAT Gateway lookup method so that it does not retrieve deleted or in deletion resources.

Migration Guide

None.

@GiamPy5 GiamPy5 force-pushed the fixes-nat-gateway-filtering branch from 86ac910 to 7ebcb5a Compare June 8, 2022 20:58
@denis256
Copy link
Member

denis256 commented Jun 9, 2022

Hi,
I'm not sure if this PR will change behavior of cloud-nuke, since it has changes only in *_test.go which is not included in the final build and it will not influence on NAT gateway filtering

aws/nat_gateway_test.go Outdated Show resolved Hide resolved
@GiamPy5
Copy link
Contributor Author

GiamPy5 commented Jun 9, 2022

Hi,

I'm not sure if this PR will change behavior of cloud-nuke, since it has changes only in *_test.go which is not included in the final build and it will not influence on NAT gateway filtering

I agree, this was just the test to confirm the bug. I wasn't able to fix it yet because I didn't have any default VPC to work with, but I was able to recreate it so I'll have the fix ready soon!

@GiamPy5 GiamPy5 changed the title [WIP] Fixes NAT gateway filtering not work Improves NAT gateway lookup function Jun 9, 2022
@GiamPy5
Copy link
Contributor Author

GiamPy5 commented Jun 9, 2022

OK so it looks like that maybe it was a problem on our side, the config file contained NATGateway instead of NatGateway, maybe it could be interesting verifying in the future if the config file contains invalid keys so that we avoid these false alarms?

Regardless, I think this PR still adds a bit of value since now I've added the test with the configuration file, and the lookup function is also more precise is not getting deleting/deleted NAT gateways.

@GiamPy5 GiamPy5 changed the title Improves NAT gateway lookup function Improves NAT gateway lookup function and adds test for config file lookup Jun 9, 2022
@GiamPy5 GiamPy5 changed the title Improves NAT gateway lookup function and adds test for config file lookup update: Skips deleted and in deletion NAT gateways from nuking, adds test for config file lookup Jun 9, 2022
@GiamPy5 GiamPy5 requested a review from denis256 June 14, 2022 14:19
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM

@yorinasub17 yorinasub17 merged commit 9d35701 into gruntwork-io:master Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants