Change prod emails from mailchimp to SES#1651
Conversation
lib/aws/ses.rb
Outdated
There was a problem hiding this comment.
We shouldn't need to provide any creds here since we can use AWS instance profiles.
With that in mind, we'll want to hold off on merging this until we have an issue resolved with devops to grant the necessary SES permissions to the IAM role associated with the EC2 instance profiles.
lib/aws/ses.rb
Outdated
lib/aws/ses.rb
Outdated
There was a problem hiding this comment.
I think we want to make this def initialize(*); end to make the linters happy
lib/aws/ses.rb
Outdated
There was a problem hiding this comment.
based on my reading of the API, destinations is not required? since the to, cc, etc are all in the raw mail body? http://docs.aws.amazon.com/ses/latest/APIReference/API_SendRawEmail.html
There was a problem hiding this comment.
You're right. For some reason I thought it was. Will fix
|
@zachmargolis: Cleaned up a bit. PTAL |
jmhooper
force-pushed
the
jmhooper-switch-to-ses
branch
from
9fbfde5 to
4827506
Compare
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM (with the caveat we should test this in a lower env to make sure it's configured correctly before merging)
lib/aws/ses.rb
Outdated
There was a problem hiding this comment.
nice trick to get around the reek complaint
jmhooper
force-pushed
the
jmhooper-switch-to-ses
branch
from
2af7a38 to
987e9fc
Compare
|
This is rebased and ready to be merged, but I have it marked |
|
Switching this to WIP so we can make this configurable based on the environment. |
|
@zachmargolis: I've modified the code in this PR so it only uses SES if the Mandrill API token is not present. Wanna take a look at that change? |
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM with the new code (once the typo is addressed)
config/environments/production.rb
Outdated
There was a problem hiding this comment.
typo? is it missing a second L? :mandrill ?
There was a problem hiding this comment.
Good catch. Fixed it in a follow up commit
brodygov
left a comment
brodygov
left a comment
There was a problem hiding this comment.
Note for deploying this, we should be sure that https://github.com/18F/identity-devops/pull/603 is rolled out to each environment prior to switching over to SES.
lib/aws/ses.rb
Outdated
There was a problem hiding this comment.
It would be good to log a message here that we're instantiating a new SES client with the given region. Ran into errors in testing this because Figaro.env.aws_region was set to "not-used-yet".
There was a problem hiding this comment.
(Fixed the "not-used-yet" stuff with https://github.com/18F/identity-devops/pull/604/ )
lib/aws/ses.rb
Outdated
There was a problem hiding this comment.
It would be good to log some message when we're about to send an email and with some data about the response we get back. The current logs are super silent, which makes diagnosing problems difficult.
|
Since we have a feature flag, I'm going to go ahead and merge this since it won't have any |
**Why**: It looks like we may need to move away from Mailchimp for compliance reasons. SES is an email service we can buy from AWS to fill the gap.
jmhooper
force-pushed
the
jmhooper-switch-to-ses
branch
from
cdd41e3 to
effdd71
Compare
3 participants
Why: It looks like we may need to move away from Mailchimp for
compliance reasons. SES is an email service we can buy from AWS to fill
the gap.