Skip to content
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 support for SESv2 under feature flag #267

Closed

Conversation

fergusdixon
Copy link

@fergusdixon fergusdixon commented Dec 15, 2022

Fixes #229

This is not adding support for any new features not previously supported by django-ses
This is just moving the current raw email logic to the new send_email method, which has some enhancements to cross-account verified identities under the hood

Personally, my motivation came from running into a similar issue to this aws/aws-cdk#18964 (comment)

New features can be added in follow up PRs

I've tested this in my own environments and works end-to-end

Copy link
Contributor

@pcraciunoiu pcraciunoiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for your work on this, and adding tests! Only small comments.

Not sure what's going on with GitHub canceling the job runs, I'll try rerunning again later. Hopefully the tests will pass.

@@ -148,6 +156,77 @@ def test_configuration_set_callable_send_mail(self):
self.assertEqual(config_set_callable.dkim_selector, 'ses')
self.assertEqual(config_set_callable.dkim_headers, ('From', 'To', 'Cc', 'Subject'))

class SESV2BackendTest(TestCase):
def setUp(self):
# TODO: Fix this -- this is going to cause side effects
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is referring to modifying django_settings? Any plans to fix this in the current PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was kinda copied from the existing tests

# TODO: Fix this -- this is going to cause side effects

https://github.com/fergusdixon/django-ses/blob/24fee37f2600ea008b296fe958404df11f21d9ec/tests/test_backend.py#L94
I feel I do a decent job cleaning up during the tearDown, so will remove the comment

django_ses/__init__.py Show resolved Hide resolved
@fergusdixon
Copy link
Author

fergusdixon commented Dec 16, 2022

Thanks so much for your work on this, and adding tests! Only small comments.

Not sure what's going on with GitHub canceling the job runs, I'll try rerunning again later. Hopefully the tests will pass.

Thanks for the comments, will work on it today
Any preference on how you want me to update the changelog? I just added what seemed reasonable but not sure how your releases work.

As for the jobs, I'm happy to update to ubuntu20 in this PR too?:
Screenshot 2022-12-16 at 08 45 21

@pcraciunoiu
Copy link
Contributor

Merged in #269, releasing as v3.3.0 now

@fergusdixon fergusdixon deleted the feature/ses-v2-feature-flag branch December 19, 2022 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support of SESv2 client
2 participants