-
Notifications
You must be signed in to change notification settings - Fork 2
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
Task tool_emailutils\task\update_suppression_list should gracefully fail #53 #54
Task tool_emailutils\task\update_suppression_list should gracefully fail #53 #54
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.
Hi Waleed,
Generally this looks like a good solution. Having it disabled by default will prevent errors where it is not being used, and having a debugging message when the configuration is not correct will help diagnose why the task is not working if it is enabled but fails.
Just a few small comments to tidy up.
a3ef213
to
0ebf2be
Compare
d0a702f
to
62876a8
Compare
@waleedhassan5 Can you always please cross link the issue number in the commit AND in the pull request description so that github does all its nice magic linking. |
62876a8
to
3da3a36
Compare
3da3a36
to
d0c7fe0
Compare
Sure sorry about that. |
…ail catalyst#53 Fixed an Issue raised about the aws suppression list update so that it doesn't throw any exceptions if it's not enabled and doesn't cause any noise.
d0c7fe0
to
a4f2305
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.
Ok, this looks good now. No noise unless the feature is enabled, and it will output a message but still succeed if the task is enabled but credentials aren't configured correctly.
Fixed an Issue #53 raised about the aws suppression list update so that it doesn't throw any exceptions if it's not enabled and doesn't cause any noise.