-
Notifications
You must be signed in to change notification settings - Fork 479
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
Added SendGrid output binding for sending emails #308
Conversation
@jjcollinge do you mind reviewing this? |
@yaron2 yh np - will do tomorrow |
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.
few comments
Removed duped line, and logging, normalised error messages
LGTM - just need to decide on whether this lives under |
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 suggest the following:
/twilio/sms.go
/twilio/sendgrid.go
Moved the sms and sendgrid bindings into a twilio folder The registration in daprd import(
"github.com/dapr/components-contrib/bindings/twilio/sms"
"github.com/dapr/components-contrib/bindings/twilio/sendgrid"
)
... ... ...
bindings_loader.NewOutput("twilio.sms", func() bindings.OutputBinding {
return sms.NewSMS(logContrib)
}),
bindings_loader.NewOutput("twilio.sendgrid", func() bindings.OutputBinding {
return sendgrid.NewSendGrid(logContrib)
}) Tested and working |
Cool, thanks |
// Licensed under the MIT License. | ||
// ------------------------------------------------------------ | ||
|
||
package sendgrid |
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.
Please open a doc issue for this as well.
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.
Just checking do you want me to set up a PR with some extra docs, or just an issue to discuss it?
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.
Doc issue to start with. PR would be great!
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.
@amanbha Hi, I've created a PR for the docs. Let me know if you need an issue to track it too dapr/docs#535
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.
Great @benc-uk Thanks. I will merge it shortly, please make the corresponding changes in dapr runtime as well.
Is sendgrid.go still under /sendgrid? It should move under /twilio/sendgrid.go |
And /twilio/sms.go |
@yaron2 I moved them around #308 (comment) |
Ahh.. just to be clear I'm not hallucinating :) Is it under /twilio/sendgrid.go? Or /twilio/sendgrid/sendgrid.go? It should be the former |
This is the convention used by all the other directories that have multiple bindings. e.g. azure, aws, gcp Otherwise we have two differently named packages in the same directory, which feels wrong |
LGTM |
Description
Added an output binding for SendGrid
This sends simple HTML emails using the SendGrid API.
Example component
The data field in the binding request is used as email body
You can specify any of the metadata properties on the request too (e.g.
emailFrom
,emailTo
,subject
)Example payload
Test / demo app is here https://github.com/benc-uk/dapr-sendgrid-test
TODO: Add docs :)
Issue reference
Fixes #314
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: