-
-
Notifications
You must be signed in to change notification settings - Fork 356
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
Implement support for SNS Topics #354
Conversation
if len(identifiers) > 100 { | ||
logging.Logger.Errorf("Nuking too many SNS Topics (100): halting to avoid hitting AWS API rate limiting") | ||
return TooManySNSTopicsErr{} | ||
} | ||
|
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.
Do we know that 100 is the limit? Should we detect rate limiting and respond to that, or is that overkill?
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.
We don't know for sure, but this is the pattern for proactively trying to avoid rate limiting that I find us carrying throughout the resources. It would be nice if we had some intelligent logic to respond to rate limiting, but I'd need to think about the cleanest way to do that.
if err == nil { | ||
logging.Logger.Infof("[OK] Deleted SNS Topic (arn=%s) in region: %s", aws.StringValue(topicArn), region) | ||
} else { | ||
logging.Logger.Errorf("[Failed] Error deleting SNS Topic (arn=%s) in %s", aws.StringValue(topicArn), region) |
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 we're rate limited, is this where execution will end up? I think this is fine. In the worst case we'll have to retry the delete.
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.
Yep, that or any other unhandled cause for an error state should end up here.
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.
LGTM except that I'm not sure about the concurrency and rate limiting. But one can always retry the command, so LGTM!
8809e8e
to
bf21784
Compare
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.
:stamp:
Thanks for reviews! Going to merge this in now. |
Description
Fixes #173 .
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Implement support for inspecting and nuking SNS Topics.
Migration Guide