-
Notifications
You must be signed in to change notification settings - Fork 727
Disable delete-protection for cognito user pool #993
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.
Let's remove these un-necessary lines & spaces.
Rest Looks good.
pkg/config/config.go
Outdated
@@ -47,6 +47,7 @@ type DisableDeletionProtection struct { | |||
CloudformationStack bool `yaml:"CloudformationStack"` | |||
ELBv2 bool `yaml:"ELBv2"` | |||
QLDBLedger bool `yaml:"QLDBLedger"` | |||
CognitoUserPool bool `yaml:"CognitoUserPool"` |
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.
CognitoUserPool bool `yaml:"CognitoUserPool"` | |
CognitoUserPool bool `yaml:"CognitoUserPool"` |
resources/cognito-userpools.go
Outdated
) | ||
|
||
type CognitoUserPool struct { | ||
svc *cognitoidentityprovider.CognitoIdentityProvider | ||
name *string | ||
id *string | ||
|
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.
resources/cognito-userpools.go
Outdated
return nil | ||
} | ||
} | ||
|
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.
Hello. I am also looking forward to have this feature in aws-nuke. Does this Merge request require additional fixes I can help with ? |
References #989 |
Nope, I believe we are all set for the referenced issue, we can get this merged. |
resources/cognito-userpools.go
Outdated
if err != nil { | ||
if f.featureFlags.DisableDeletionProtection.CognitoUserPool{ | ||
err = f.DisableProtection() | ||
if err!=nil{ |
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.
Can you please quickly run go fmt
on these files?
resources/cognito-userpools.go
Outdated
svc *cognitoidentityprovider.CognitoIdentityProvider | ||
name *string | ||
id *string | ||
featureFlags config.FeatureFlags |
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.
Formatting is still off here. You should setup your IDE to use gofmt
to automatically format files on save.
resources/cognito-userpools.go
Outdated
} | ||
userPool := userPoolOutput.UserPool | ||
params := &cognitoidentityprovider.UpdateUserPoolInput{ | ||
DeletionProtection: &cognitoidentityprovider.DeletionProtectionType_Values()[1], |
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.
DeletionProtection: &cognitoidentityprovider.DeletionProtectionType_Values()[1], | |
DeletionProtection: aws.String(cognitoidentityprovider.DeletionProtectionTypeInactive), |
Using the constant here instead of accessing an array by numbers
resources/cognito-userpools.go
Outdated
_, err := f.svc.DeleteUserPool(&cognitoidentityprovider.DeleteUserPoolInput{ | ||
UserPoolId: f.id, | ||
}) | ||
if err != nil { | ||
if f.featureFlags.DisableDeletionProtection.CognitoUserPool { |
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.
if f.featureFlags.DisableDeletionProtection.CognitoUserPool { | |
awsErr, ok := err.(awserr.Error) | |
if ok && strings.Contains(awsErr.Message(), "Deletion protection must be inactivated first") && | |
f.featureFlags.DisableDeletionProtection.CognitoUserPool { | |
I'd prefer checking here if the error is actually caused by the Deletion Protection before proceeding
Ran into this issue today. It looks like this is pending some updates from the author @suleman-sohail. If they're unable to get to this, should a new PR be opened? It doesn't look like much is left here but some cosmetic changes. |
Hi, I could not reply anytime soon, thanks for waiting. I believe I was able to address the relevant comments, I might have miss something. You can create PR on child branch of this one, do any changes you think are needed for this PR to get merged, get that merged in this one, and we are good to go. |
This was implemented in the fork via this PR ekristen/aws-nuke#311 and is in 3.23.0 and forward. If you have a chance, please check it out and let us know if you run into an issues by opening an issue over on the fork. Please see the copy of the notice from the README about the deprecation of this project. Sven was kind enough to grant me access to help triage and close issues and pull requests that have already been addressed in the actively maintained fork. Some additional information is located in the welcome issue for more information. Caution This repository for aws-nuke is no longer being actively maintained. We recommend users to switch to the actively maintained fork of this project at ekristen/aws-nuke. |
On nuking aws account, having cognito user pool, as one of its resources, nuke fails to cleanup.
deletionProtection
support is added in remove method of cognito-user-pool.