search

ekristen / aws-nuke

Remove all the resources from an AWS account
https://ekristen.github.io/aws-nuke/
MIT License
32 stars 6 forks source link

feat: adding QuickSight resource #180

Closed danielbojczuk closed 1 month ago

ekristen commented 1 month ago

Welcome @danielbojczuk! I was just working on a PR to pull this over last night and you beat me to it! Even better you added some tests too!

Looks like there are a few things that I would prefer to get addressed now before merge. Things like streamlining a few variable names and adjusting how the termination protection is removed from the resource. I'm going to do a PR review and comment on all the lines, if you'd like I can make a commit to the PR but wanted to give you the opportunity to author them all if you like.

danielbojczuk-jumbo commented 1 month ago

Hi @ekristen, I really appreciate your response.

I would love to work on the improvement points from your PR review! 😀

danielbojczuk commented 1 month ago

@danielbojczuk Again thanks for the submission!

I'm realizing now I need to document the process better for adding new resources. 90% of these comments are suggestions you can just accept, the only one that is manual is the Setting comment.

If you'd rather I can do a commit on top of your PR to make the changes. I'm committed to getting this in, so let me know your preference.

Thanks!!! I will accept the suggestions and work on the termination protection today or tomorrow.

danielbojczuk commented 1 month ago

@danielbojczuk one more small set of changes. First one is a mismatch on pointers on the new function you added and the second set is just a code cleanliness nit, matching case to the rest of QuickSight in the file. Other than that we are good to go! Tests are passing.

Thanks @ekristen. I accepted the suggestions.

Thanks for your quick review and work. This feature will be handy for us!

ekristen commented 1 month ago

Normally I would have just done the fixes, but you seemed like you wanted to do it all. If you want to rebase and force push a single commit with everything as your final step +1, it's 100% not required, I don't mind the extra commits.

danielbojczuk commented 1 month ago

Normally I would have just done the fixes, but you seemed like you wanted to do it all. If you want to rebase and force push a single commit with everything as your final step +1, it's 100% not required, I don't mind the extra commits.

Hi @ekristen, Thanks! This is actually my first code in Go, apart from a "hello world." So, doing these fixes helps me learn about best practices and language styles. I did a rebase only for the latest style fixes.

I installed and ran golangci-lint locally. Seems to be ok now :)

ekristen commented 1 month ago

Welcome to the wonderful world of go. No better language in my opinion. This project is highly opinionated and so are all my projects. Thanks for the changes and the rebase. Looks like all tests passed, so merge inbound.

ekristen commented 1 month ago

:tada: This PR is included in version 3.0.0-beta.55 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

ekristen commented 1 month ago

I should be removing the beta tag soon. I have a few things to finish up around documentation, then 3.0.0 will get officially dropped.

ekristen commented 1 month ago

For reference, this was the original upstream. https://github.com/rebuy-de/aws-nuke/pull/1228

ekristen commented 4 weeks ago

:tada: This PR is included in version 3.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: