-
Notifications
You must be signed in to change notification settings - Fork 38
feat: Added SMTP extension #159
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
Conversation
Signed-off-by: Daniel Liszka <daniel@chainloop.dev>
Signed-off-by: Daniel Liszka <daniel@chainloop.dev>
Signed-off-by: Daniel Liszka <daniel@chainloop.dev>
Signed-off-by: Daniel Liszka <daniel@chainloop.dev>
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.
Awesome, good start!
I left some comments, most of them suggestions that can be changed in another patch.
For example, by encapsulating the render code in a function + go templates you'll be able to test them.
I'd suggest to also add comments in the code explaining/separating the different blocks. And as a general rule, make sure that initialization of variables/code happens the closest to the code that'll use it as possible (i.e preparing the response in register
.
Looking good!
From string `json:"from" jsonschema:"format=email,description=The email address of the sender."` | ||
User string `json:"user" jsonschema:"minLength=1,description=The username to use for the SMTP authentication."` | ||
Password string `json:"password" jsonschema:"description=The password to use for the SMTP authentication."` | ||
Host string `json:"host" jsonschema:"description=The host to use for the SMTP authentication."` |
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.
Would any of these format help? https://json-schema.org/understanding-json-schema/reference/string.html#hostnames
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.
This is not supported by our jsonschema library - I can work on the PR though. We would have to use something like: anyof_format=hostname,ipv4
to get
"anyOf": [
{ "format": "hostname" },
{ "format": "ipv4" },
{ "format": "ipv6" }
]
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 see, that makes sense. It's not just a host but also an IP address.
return nil | ||
} | ||
|
||
func validateExecuteRequest(req *sdk.ExecutionRequest) error { |
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.
mental note for myself: this is starting to look more and more like smth that could get automatically validated.
}, | ||
{ | ||
name: "valid request", | ||
input: map[string]interface{}{"from": "test@example.com", "to": "test@example.com", "host": "localhost", "port": "25", "user": "test", "password": "test"}, |
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.
probably test email formats and potential hostnames.
Signed-off-by: Daniel Liszka <daniel@chainloop.dev>
Please link the issue in the description not in the title of the PR. |
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.
LGTM!
From string `json:"from" jsonschema:"format=email,description=The email address of the sender."` | ||
User string `json:"user" jsonschema:"minLength=1,description=The username to use for the SMTP authentication."` | ||
Password string `json:"password" jsonschema:"description=The password to use for the SMTP authentication."` | ||
Host string `json:"host" jsonschema:"description=The host to use for the SMTP authentication."` |
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 see, that makes sense. It's not just a host but also an IP address.
This is a new extension for sending emails via SMTP. This is still a very early experimental version focusing on validating the latest cutting-edge integrations framework.
The README file provides more information on how to use this extension.
closes #158