-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
SES: implement SetIdentityHeadersInNotificationsEnabled API #12823
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
SES: implement SetIdentityHeadersInNotificationsEnabled API #12823
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.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
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.
Thanks for creating ths PR! Sorry for the delayed review. I have some points that need to be addressed before we can merge this.
…NotificationsEnabled
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.
Thanks for adding the additional validations. There's a small issue with the tests, once addressed, we can merge this.
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.
Looks pretty good already @YelynnOh. I have last few nits and we can merge this
@YelynnOh There are some linting failures that are preventing the pipeline from completing. They should be simple to resolve by running |
@viren-nadkarni I ran |
@viren-nadkarni Thankyou for adjusting the arguments. I have added |
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.
Looks great! Congratulations on your first contribution to LocalStack!
Motivation
close #12516
The issue was caused because the
SetIdentityHeadersInNotificationsEnabled
API was not implemented in LocalStack’s SES provider, resulting in failures when this API was called.This PR adds the
SetIdentityHeadersInNotificationsEnabled
API.The reference for this api is written below, if needed.
https://docs.aws.amazon.com/ses/latest/APIReference/API_SetIdentityHeadersInNotificationsEnabled.html
Changes
SetIdentityHeadersInNotificationsEnabled
handler inlocalstack-core/localstack/services/ses/provider.py
.tests/aws/services/ses/test_ses.py
to verify the new API.test_ses.snapshot.json
is also added.