Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

CLOUD-1310 adds rds cluster-related resources #48

Merged
merged 1 commit into from
Nov 20, 2017

Conversation

octopusx
Copy link
Contributor

This commit enables aws-nuke to recognise DBClusterParameterGroup and
DBCluster type resources and remove them.
The changes are necessary to nuke Aurora clusters.

@rebuy-de/prp-aws-nuke please review

This commit enables aws-nuke to recognise DBClusterParameterGroup and
DBCluster type resources and remove them.
The changes are necessary to nuke Aurora clusters.
}

_, err := i.svc.DeleteDBClusterParameterGroup(params)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you just skip this if and directly: return err

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO the logic is more understandable and separated like this...
first: error handling
then: making the function return what is should

Copy link
Member

Choose a reason for hiding this comment

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

IMO it looks like unnecessary code :>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could, but then you are implying that whoever looks at the code afterwards understands that err defaults to nil. The if statement makes it more explicit and easier to read what's going on. Also, this is how that error is handled across all the classes in aws-nuke where it's applicable, so it is following a common pattern in the program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decision?

Copy link
Member

Choose a reason for hiding this comment

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

Your call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will leave it in for consistency's sake. If this is a big enough problem then it deserves its own pull request and that if statement should be removed everywhere across the whole application.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very much agree. Greatly in favor of consistency.

@octopusx octopusx merged commit 8399681 into master Nov 20, 2017
@octopusx octopusx deleted the add_rds_cluster_resources branch November 20, 2017 16:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants