-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Nuke cloudwatch dashboard #233
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! I haven't had a chance to give it a test, but I assume you'll try running it against PHX anyway very soon.
I've left some questions - those are there for me to be able to build better knowledge and mental model of cloud-nuke in general, and then some NITs - mostly around formatting, so feel free to ignore too.
includeREs := []Expression { { RE: *include } } | ||
includeREs := []Expression{{RE: *include}} | ||
|
||
exclude, err := regexp.Compile(`.*openvpn.*`) | ||
require.NoError(t, err) | ||
excludeREs := []Expression { { RE: *exclude } } | ||
excludeREs := []Expression{{RE: *exclude}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: just noting if this was intentional - I don't think it's a massive problem, but some different formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was sort of intentional. It happened when I ran goimports
on the file. I think it may have to do with a go version change (I recently upgraded to go 1.17).
config/config.go
Outdated
} else if matches(name, excludeREs) { | ||
return false | ||
// Given the 'name' is not in the 'exclude' list, should include if there is no 'include' list | ||
// Given the 'name' is not in the 'exclude' list, should include if there is no 'include' list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: same here - seems to have added some extra spacing before the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yea this is not good. Again, I think this was caused by my upgrade to go 1.17. It looks like this is now the new standard formatting. I moved the comments around to make it more readable: f1cf960
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect! thanks for addressing it
return 100 | ||
} | ||
|
||
// Nuke - nuke 'em all!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😁
return nil | ||
} | ||
|
||
// NOTE: we don't need to do pagination here, because the pagination is handled by the caller to this function, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to confirm - is that a pattern we should adopt more or less in all cases for pagination?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we should. I've been doing this on any new resource I introduce, but haven't been going back and retroactively porting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that's awesome! I've raised an issue actually some weeks ago about this: #226 so really good to see this!
aws/cloudwatch_dashboard_test.go
Outdated
cwdbName := createCloudWatchDashboard(t, svc, region) | ||
defer deleteCloudWatchDashboard(t, svc, cwdbName, true) | ||
|
||
// Assert CWDB is picked up without filters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: could we maybe swap this comment abbreviation out for the full name? Just to keep it easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! cdaafce
|
||
cwdbNames := []*string{} | ||
for i := 0; i < 3; i++ { | ||
// We ignore errors in the delete call here, because it is intended to be a stop gap in case there is a bug in nuke. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: for my knowledge - so this stop gap is meant to try & delete the dashboard once, but if that call fails, then it leaves it at that in case the deletion failed for a good reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no. So the logic here is that the test makes a call to nukeAllCloudWatchDashboards
, which will go ahead and delete the dashboards that it created in the test, if it works. However, if there is a bug in the function and it fails, those dashboards will be lying around. To avoid having the test pollute the environment with those dashboards, we attempt an alternative cleaning function here to try to make sure those get deleted.
The reason we can't error check is that in the happy path, the dashboards would be deleted by the nuke function and they won't exist, thus this function will throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh! that makes total sense! thanks, Yori :)
Ah I already ran it. I wrote this because I found a bunch of dashboards across multiple regions lying around in PHX and wanted to easily delete them, while also having a more semi-permanent way to periodically delete them. |
OK, that's great! All looks good now, so approving 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
(Caveat: the failing test doesn't seem to be linked to this new feature, so all good there, we can handle it separately)
Thanks for thorough review! Merging and releasing now! |
Fixes #232