-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add error handling around SNS API calls #21
Conversation
@sns_topic_arns = fetch_sns_topic_arns | ||
render :new | ||
else | ||
@sns_subscription.save |
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.
Use save!
if you don't check the return value
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.
fixed.
@@ -50,6 +61,8 @@ def destroy | |||
|
|||
def fetch_sns_topic_arns | |||
sns_client.list_topics.topics.map(&:topic_arn) | |||
rescue Aws::Errors::MissingCredentialsError, Aws::SNS::Errors::AuthorizationError |
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.
I think these exceptions should be reported as 5xx error because sns:ListTopics permission is required in Barbeque
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.
I think so, too. fixed.
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.
Also having tests for AuthorizationError may be better.
done. I'll release v0.4.1. Thank you! |
I add error handling around SNS API calls.
In our environment we want to restrict available topics to subscribe by IAM roles. So if a user is setting a restricted topic, Barbeque returns 500 error for now.
This patch avoids 500 error and shows error messages when accessing unauthorized topic or not found.
@cookpad/dev-infra @k0kubun How do you think?