Upgrade from boto to boto3, and use env variables #183

GitRon · 2020-05-29T06:59:52Z

Hi guys,

I started the migration from boto to boto3.

I tried it out locally and now I can send from an email region which has this new auth type v4.

Here's a list of stuff I already did:

Upgrade from boto to boto3
Some flake8 linting
Fixed both management command
Sending mails from eu-central-region works now
Removed proxy and updated docs

Fixes #181 #78

pcraciunoiu · 2020-05-29T17:29:33Z

@GitRon thanks for starting on this! The build fails, would be good to fix.

Could you post this in the relevant project issues? That way others can find it and try to contribute as well.

GitRon · 2020-06-02T06:32:42Z

@GitRon thanks for starting on this! The build fails, would be good to fix.

Could you post this in the relevant project issues? That way others can find it and try to contribute as well.

I added a list in the PR to all known issues we have to deal with. I'll have a look at the tests, at least this is my responsibility ;)

And just got the info that the issue got a new level of urgency: To improve the security for our customers, beginning October 1, 2020, Amazon Signature Version 3 will be turned off (deprecated) in Amazon SES in favor of Signature Version 4.

trecouvr

Thank you GitRon, I am highly interested by your patch since the new announcement of AWS regarding the drop of signature V3.

I reviewed your PR and travis fails because you introduced 2 new requirements in setup.py :

python_requires='>=3.5.8',
install_requires=[..., "django>1.11"],

If you look at the travis matrix, it starts at python2 and Django 1.11. You could try to remove these requirements (I added a patch suggestion to this comment) and see if the build pass. Otherwise there may be a need to drop support for oldest versions.

trecouvr · 2020-06-02T12:40:59Z

setup.py

    packages=find_packages(exclude=['example', 'tests']),
    package_data=package_data,
+    python_requires='>=3.5.8',


Suggested change

python_requires='>=3.5.8',

thanks so much for your input! I changed the python version to 3.4.10 and removed python 2.7 in the travix matrix. After all, python 2.7 is dead. Long live python 3.

I'm fine dropping support for python 2, though if it's not a big deal we could keep it (I suspect some users will be affected). However it's not worth a lot of time... And hopefully not too many users complain.

It would be great if you could add the supported Django versions to the README. As far as that, I'd be fine supporting 1.10 or even 1.11 and above. Django 3 should be easy if 2.2 works...

Ok, when the tests are fixed, I'll give Python 2.7 another shot and I added django 1.10 to the tox and travis.

trecouvr · 2020-06-02T12:41:17Z

setup.py

@@ -142,7 +141,7 @@ def find_package_data(where=".", package="", exclude=standard_exclude,
    long_description=LONG_DESCRIPTION,
    platforms=['any'],
    classifiers=CLASSIFIERS,
-    install_requires=["boto>=2.31.0", "pytz>=2016.10", "future>=0.16.0"],
+    install_requires=["boto3>=1.0.0", "pytz>=2016.10", "future>=0.16.0", "django>1.11"],


Suggested change

install_requires=["boto3>=1.0.0", "pytz>=2016.10", "future>=0.16.0", "django>1.11"],

install_requires=["boto3>=1.0.0", "pytz>=2016.10", "future>=0.16.0"],

Why did you remove the django dependency here?

I removed it because because Travis matrix has Django 1.11 (https://github.com/django-ses/django-ses/blob/master/.travis.yml#L12) and because it was not on the initial source (https://github.com/django-ses/django-ses/blob/master/setup.py#L145), but maybe it's a good thing to add it?

I mainly added it, because the IDE gives you loades of warnings if it's missing 😅

…nd updated readme

pcraciunoiu

If we do keep support for python 2.7, I wonder if it would be easy to add a deprecation warning for it, saying this will be the last release to support it. See this article for a bit on warnings if you're new to them: https://lerner.co.il/2020/04/27/working-with-warnings-in-python/

GitRon · 2020-06-05T11:01:24Z

If we do keep support for python 2.7, I wonder if it would be easy to add a deprecation warning for it, saying this will be the last release to support it. See this article for a bit on warnings if you're new to them: https://lerner.co.il/2020/04/27/working-with-warnings-in-python/

Seems like a good idea. I'll look into it when we know for sure that we'll stick to python 2.7

GitRon · 2020-06-05T13:23:51Z

We found this post stating we don't need a proxy anymore (which has been removed in boto3 anyway) and you can do it via the ENV vars. Anybody disagree if we remove it?

I just added the dkim again, though I can't test it, it might just as well work because it's an external package.

pcraciunoiu · 2020-06-05T15:38:57Z

Yeah that seems fine to remove if it can work via ENV vars.

Though if it is a separate change, you can also do it in the future/separate PR. Whichever is easier?

chrisconlan · 2020-06-08T13:53:19Z

Following this because of the Signature V4 deadline. Thanks for all your great work!

GitRon · 2020-06-09T11:19:08Z

Yeah that seems fine to remove if it can work via ENV vars.

Though if it is a separate change, you can also do it in the future/separate PR. Whichever is easier?

I removed the proxy logic and added a part to the documentation.

GitRon · 2020-06-09T11:21:08Z

Ok, update on my progress: dkim is back in, the package seems not to be related to the boto3 upgrade. Though I cannot test it, it still just keep working.

Still struggling with some tests, working on mocking the send_raw_mail function properly... That's my current task.

trecouvr · 2020-06-09T13:33:21Z

Hi,

I fixed the tests based on GitRon work. Everything passes except the Python 2.7 tests because of the requirements : https://travis-ci.org/github/django-ses/django-ses/builds/696453533.

Also I used a different approach than yours GitRon to mock the different classes, this result in less code diff between the PR and the current master.

Hope this solves most of issues. I made a PR to your repo GitRon, so you can integrate my changes with just one click ;).

Best,

GitRon · 2020-06-09T14:06:33Z

Hi,

I fixed the tests based on GitRon work. Everything passes except the Python 2.7 tests because of the requirements : https://travis-ci.org/github/django-ses/django-ses/builds/696453533.

Also I used a different approach than yours GitRon to mock the different classes, this result in less code diff between the PR and the current master.

Hope this solves most of issues. I made a PR to your repo GitRon, so you can integrate my changes with just one click ;).

Best,

@trecouvr You are the best! ❤️ I have some questions about your fix - if you might clarfiy it, then I'll merge it gratefully into my branch!

Fix tests

README.rst

django_ses/__init__.py

setup.py

pcraciunoiu

Looks good to me. At this point my main concern is manual testing, if someone could confirm this works for them, stats included :)

GitRon · 2020-06-10T19:08:47Z

@pcraciunoiu Fixed your comments, will test all that I can test on Friday. Hopefully @trecouvr finds time for testing as well 😃

pcraciunoiu · 2020-06-10T19:11:34Z

@GitRon sounds good! Thanks for doing this!!

trecouvr · 2020-06-11T15:18:15Z

Hi, I'll make some time to test it through our applications using it. But we only use the send part of this library, never used a proxy, dkim or the stats.

trecouvr · 2020-06-11T16:01:23Z

Maybe the PR #73 could be merged, then we can ask @brutasse to re-test the dkim part with this branch?

GitRon · 2020-06-12T06:04:14Z

@GitRon sounds good! Thanks for doing this!!

I successfully sent emails via eu-west and eu-central - so I guess it works now 🚀

GitRon · 2020-06-12T06:07:07Z

I like the idea of getting dkim fixed as well but the branch is quite old and we need the base functionality going. so if we don't get a reply soon, I suggest we throw out version 1.0.0 and can always add new bugfix releases if some of the minor functionality is broken. what do you think about this approach?

django_ses/management/commands/get_ses_statistics.py

trecouvr · 2020-06-12T08:28:59Z

Ok I managed to send emails and get the 2 managements commands working (needed a change for the statistics, but I dont think this is related to the ongoing work, see my comment above).

As for the DKIM part, not sure how many people use it, I use what you call in your doc "Easy DKIM" which allows SES to sign directly the emails.

pcraciunoiu · 2020-06-12T14:56:19Z

@GitRon @trecouvr so does this seem ready from your end? I might wait until Monday or do a release on the weekend, but wanted to confirm it's good to go. I'll try and re-review soon too, but I imagine it's in good shape now :)

GitRon · 2020-06-15T06:30:26Z

@pcraciunoiu The stuff I need now works. About all the things which are included in this package... maybe there should be a reevaluation, if we still need to carry it all around. Like dkim, I think there is a AWS way of doing it.
Really looking forward to the release! 🎉

pcraciunoiu · 2020-06-15T16:47:41Z

django_ses/__init__.py

-                                          dkim_headers=self.dkim_headers)
+                    Source=source or message.from_email,
+                    Destinations=message.recipients(),
+                    # todo attachments?


@GitRon were you planning on doing this, is it worth highlighting in a changelog?

Good point! I actually wanted to test attachements to make sure they would work but totally forgot about it! I'll check it asap.

pcraciunoiu · 2020-06-15T17:00:27Z

Made a couple small changes (the version parsing wasn't quite right -- needed to be joined into a string).

Release created: https://github.com/django-ses/django-ses/releases/tag/v1.0.0

And pushed to pypi: https://pypi.org/project/django-ses/1.0.0/

Please test it out. Can do a patch release if stats is broken.

pcraciunoiu · 2020-06-15T17:04:55Z

@pcraciunoiu The stuff I need now works. About all the things which are included in this package... maybe there should be a reevaluation, if we still need to carry it all around. Like dkim, I think there is a AWS way of doing it.

Yeah I would be glad to remove stuff that is supported in different ways.

GitRon · 2020-06-16T06:30:51Z

@pcraciunoiu The stuff I need now works. About all the things which are included in this package... maybe there should be a reevaluation, if we still need to carry it all around. Like dkim, I think there is a AWS way of doing it.

Yeah I would be glad to remove stuff that is supported in different ways.

Maybe we could open an issue for it? So it won't be forgotten?

pcraciunoiu · 2020-06-16T15:04:17Z

@pcraciunoiu The stuff I need now works. About all the things which are included in this package... maybe there should be a reevaluation, if we still need to carry it all around. Like dkim, I think there is a AWS way of doing it.

Yeah I would be glad to remove stuff that is supported in different ways.

Maybe we could open an issue for it? So it won't be forgotten?

Yeah want to go ahead and open one, listing what you have in mind?

GitRon · 2020-06-19T06:59:06Z

@pcraciunoiu The stuff I need now works. About all the things which are included in this package... maybe there should be a reevaluation, if we still need to carry it all around. Like dkim, I think there is a AWS way of doing it.

Yeah I would be glad to remove stuff that is supported in different ways.

Maybe we could open an issue for it? So it won't be forgotten?

Yeah want to go ahead and open one, listing what you have in mind?

Nothing in particular, just maybe look at all the functionality and management commands I personally never use and check if they a) work and b) are still required.

I just tested sending emails with attachments - it works! So my todo was unnecessary!

Ronny Vedrilla added 3 commits May 27, 2020 10:38

Upgrade to boto3

814ed5c

Upgrade to boto3

b7b2f2e

Upgrade to boto3

8b0abd4

This was referenced May 29, 2020

InvalidClientTokenId with "AWS_SES_REGION_NAME = 'eu-central-1'" #181

Closed

Support for Boto3 #78

Closed

Added some badges to README.rst

cf5f1f8

trecouvr reviewed Jun 2, 2020

View reviewed changes

trecouvr mentioned this pull request Jun 2, 2020

Need to update the signature version for authentication? #184

Closed

Ronny Vedrilla added 5 commits June 2, 2020 14:50

Adjust travis matrix and changed pyton dependency to match travis

971c8f1

Adjusted python version to match travis

03c2233

Fixed first test

8fede94

Fixed test_commands tests

e20388a

Added python 2.7 for travis again to see if we have to drop support a…

00b9390

…nd updated readme

pcraciunoiu reviewed Jun 3, 2020

View reviewed changes

Fixed some tests, removed proxy, added docs about proxy alternative

451b7af

trecouvr mentioned this pull request Jun 9, 2020

Help on fixing tests for Feature/boto3 #185

Closed

Ronny Vedrilla and others added 4 commits June 9, 2020 16:08

Downgraded python version for travis

7309383

Fix tests

b21ac79

Merge pull request #1 from trecouvr/feature/boto3

f14d281

Fix tests

Partial flake8 Linting

56132c7

pcraciunoiu reviewed Jun 10, 2020

View reviewed changes

README.rst Outdated Show resolved Hide resolved

pcraciunoiu reviewed Jun 10, 2020

View reviewed changes

django_ses/__init__.py Outdated Show resolved Hide resolved

pcraciunoiu reviewed Jun 10, 2020

View reviewed changes

setup.py Show resolved Hide resolved

pcraciunoiu reviewed Jun 10, 2020

View reviewed changes

Feedback fixes for second review

d1cbca8

pcraciunoiu mentioned this pull request Jun 11, 2020

Fix DKIM signing on python 3 #73

Open

trecouvr reviewed Jun 12, 2020

View reviewed changes

django_ses/management/commands/get_ses_statistics.py Show resolved Hide resolved

Included improvement by @trecouvr

34cfb78

pcraciunoiu changed the title ~~Feature/boto3~~ Upgrade from boto to boto3, and use env variables Jun 15, 2020

pcraciunoiu reviewed Jun 15, 2020

View reviewed changes

pcraciunoiu approved these changes Jun 15, 2020

View reviewed changes

pcraciunoiu merged commit 09b2065 into django-ses:master Jun 15, 2020

GitRon deleted the feature/boto3 branch June 16, 2020 06:30

trecouvr mentioned this pull request Jun 25, 2020

Differences with django-ses? azavea/django-amazon-ses#8

Open

cocococosti mentioned this pull request Mar 2, 2021

Updating django-ses version eduNEXT/edx-platform#203

Merged

awais786 mentioned this pull request Feb 19, 2023

Replace boto-based exceptions from bulk_email openedx/edx-platform#31779

Closed

	install_requires=["boto3>=1.0.0", "pytz>=2016.10", "future>=0.16.0", "django>1.11"],
	install_requires=["boto3>=1.0.0", "pytz>=2016.10", "future>=0.16.0"],

Upgrade from boto to boto3, and use env variables #183

Upgrade from boto to boto3, and use env variables #183

Conversation

GitRon commented May 29, 2020 • edited Loading

pcraciunoiu commented May 29, 2020

GitRon commented Jun 2, 2020

trecouvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcraciunoiu left a comment

Choose a reason for hiding this comment

GitRon commented Jun 5, 2020

GitRon commented Jun 5, 2020 • edited Loading

pcraciunoiu commented Jun 5, 2020 • edited Loading

chrisconlan commented Jun 8, 2020

GitRon commented Jun 9, 2020

GitRon commented Jun 9, 2020

trecouvr commented Jun 9, 2020 • edited Loading

GitRon commented Jun 9, 2020

pcraciunoiu left a comment

Choose a reason for hiding this comment

GitRon commented Jun 10, 2020

pcraciunoiu commented Jun 10, 2020

trecouvr commented Jun 11, 2020

trecouvr commented Jun 11, 2020

GitRon commented Jun 12, 2020

GitRon commented Jun 12, 2020

trecouvr commented Jun 12, 2020

pcraciunoiu commented Jun 12, 2020

GitRon commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcraciunoiu commented Jun 15, 2020

pcraciunoiu commented Jun 15, 2020

GitRon commented Jun 16, 2020

pcraciunoiu commented Jun 16, 2020

GitRon commented Jun 19, 2020

GitRon commented May 29, 2020 •

edited

Loading

GitRon commented Jun 5, 2020 •

edited

Loading

pcraciunoiu commented Jun 5, 2020 •

edited

Loading

trecouvr commented Jun 9, 2020 •

edited

Loading