-
Notifications
You must be signed in to change notification settings - Fork 0
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 basic email service #4
Conversation
ProcessorFunction: | ||
Type: AWS::Serverless::Function | ||
Properties: | ||
FunctionName: !Sub anghammarad-${Stage} | ||
Description: !Sub Process Anghammarad ${Stage} notifications | ||
Role: !GetAtt AnghammaradRole.Arn |
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.
Might be easier to just provide the Policy
, rather than the entire Role
? If you give a Policies
attribute all the policies you provide will be added to the default Lambda role so you only need to add the permissions that are being added, not the rest of the role (e.g. the assume role bit)
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.
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 - less YAML is better
@@ -33,10 +33,10 @@ object Anghammarad { | |||
} | |||
} | |||
|
|||
def send(contact: Contact, message: Message): Try[Unit] = { | |||
def send(contact: Contact, message: Message): Try[SendEmailResult] = { |
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.
I've changed this stuff a bit in that other PR and elsewhere, sorry. Let's do some work to merge what you have into the latest version.
|
||
val awsMessage = new AwsMessage() | ||
.withSubject(buildContent(message.subject)) | ||
.withBody(new Body().withHtml(buildContent(message.contents))) |
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.
Does it need a plain text body as a fallback as well? (Great if not!)
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.
I've added a text version for now...
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, thank you!
.withSource("foo123@theguardian.com") //TODO read from environment variable or conf | ||
.withMessage(awsMessage) | ||
|
||
} |
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.
Formatting is a bit off here but no dramas, we can tidy up as we go.
|
||
object EmailService { | ||
|
||
private val provider = new AWSCredentialsProviderChain( |
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.
As you said elsewhere, at some point we should pull this out so it can be used for other functionality as well.
Adds a basic email service and (hopefully) the permissions required to send emails via SNS when running locally and in a lambda.