Clean up settings and add new configuration parameters #36

Merged
merged 3 commits into from Feb 21, 2012

2 participants

@dlo

There are two commits in this pull request. The first just consolidates all the settings we're using from Django into a single file. The second creates two new settings values, AWS_SES_ACCESS_KEY_ID and AWS_SES_SECRET_ACCESS_KEY. We need this in production because we use different access keys for different AWS services (this makes key invalidation a bit easier in case anything is compromised, and lets us track usage much more granularly).

I updated the documentation as well. No new tests needed to be written because internally the library still uses AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY.

Enjoy!

@tswicegood tswicegood and 1 other commented on an outdated diff Feb 20, 2012
django_ses/__init__.py
@@ -1,5 +1,5 @@
from django.core.mail.backends.base import BaseEmailBackend
-from django.conf import settings
+import settings
@tswicegood
tswicegood added a line comment Feb 20, 2012

This should be from . import settings to avoid accidentally importing a root level settings module.

@tswicegood
tswicegood added a line comment Feb 20, 2012

Alternatively, you could also do from django_ses import settings.

@dlo
dlo added a line comment Feb 20, 2012

Good call, I'll fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tswicegood tswicegood and 1 other commented on an outdated diff Feb 20, 2012
django_ses/settings.py
+from django.conf import settings
+from boto.ses import SESConnection
+
+DKIM_DOMAIN = getattr(settings, "DKIM_DOMAIN", None)
+DKIM_PRIVATE_KEY = getattr(settings, 'DKIM_PRIVATE_KEY', None)
+DKIM_SELECTOR = getattr(settings, 'DKIM_SELECTOR', 'ses')
+DKIM_HEADERS = getattr(settings, 'DKIM_HEADERS',
+ ('From', 'To', 'Cc', 'Subject'))
+
+AWS_ACCESS_KEY_ID = getattr(settings, 'AWS_SES_ACCESS_KEY_ID', None)
+if not AWS_ACCESS_KEY_ID:
+ AWS_ACCESS_KEY_ID = getattr(settings, 'AWS_ACCESS_KEY_ID', None)
+
+AWS_SECRET_ACCESS_KEY = getattr(settings, 'AWS_SES_SECRET_ACCESS_KEY', None)
+if not AWS_SECRET_ACCESS_KEY:
+ AWS_SECRET_ACCESS_KEY = getattr(settings, 'AWS_SECRET_ACCESS_KEY', None)
@tswicegood
tswicegood added a line comment Feb 20, 2012

These lines here could be condensed a bit further:

AWS_SECRET_ACCESS_KEY = getattr(settings, "AWS_SES_SECRET_ACCESS_KEY",
    getattr(settings, "AWS_SECRET_ACCESS_KEY", None))

As a side note, shouldn't there be tests around this behavior to make sure that the correct keys are picked up and in the correct order?

@dlo
dlo added a line comment Feb 20, 2012

I did that at first, but wasn't sure if it was more readable. Thoughts? I can change it.

@tswicegood
tswicegood added a line comment Feb 20, 2012

I prefer the terser output, but that's 100% only the color of the bike shed that appeals most to me. :-)

One thing that's throwing me though, as I read it is that we put the AWS_SES_SECRET_ACCESS_KEY in the value AWS_SECRET_ACCESS_KEY. Any thoughts about dropping the AWS_ prefix on the internal variable so we only have SECRET_ACCESS_KEY and ACCESS_KEY_ID? Or maybe even shorter, SECRET_KEY and ACCESS_KEY?

@dlo
dlo added a line comment Feb 20, 2012

Ok cool, I'll change that.

Agreed on the shorter settings version, it is kind of confusing when they're both the same name. I'll go with SECRET_KEY and ACCESS_KEY if that's alright.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tswicegood

This looks good with the exception of tests. We should really make sure that the django_ses.settings module is doing what's expected of it. Think of it as an early warning system for future devs should forget and/or don't pay attention to the way it's documented and try to change it.

@dlo

Great, I'll add some tests to make sure that django_ses.settings is pulling in the correct values.

dlo added some commits Feb 20, 2012
@dlo dlo consolidate all SES settings to single file
This should make maintenance a bit easier.
7207c50
@dlo dlo add AWS_SES_ACCESS_KEY_ID and AWS_SES_SECRET_ACCESS_KEY to settings file
Include updated documentation as well. No new tests are needed.
acf8a35
@dlo dlo add tests for new settings values 56f6e81
@tswicegood tswicegood merged commit 56f6e81 into django-ses:master Feb 21, 2012
@tswicegood

I moved the tests.configuration to tests.settings, but otherwise we're set. Thanks!

@pcraciunoiu pcraciunoiu commented on the diff Feb 21, 2012
django_ses/tests/utils.py
@@ -0,0 +1,5 @@
+import sys
+
+def unload_django_ses():
@pcraciunoiu
pcraciunoiu added a line comment Feb 21, 2012

Neat stuff. Thanks to both of you for contributing. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment