Skip to content
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

Implement default email sender #207

Merged
merged 14 commits into from
Jun 21, 2024

Conversation

santihernandezc
Copy link
Contributor

@santihernandezc santihernandezc commented Jun 18, 2024

Description

This PR adds a default email sender that implements the EmailSender interface.
It also includes a function that returns a factory function that can be passed to BuildReceiverIntegrations when creating Grafana receivers.

This can be smoke-tested using this Mimir PoC.

@santihernandezc santihernandezc force-pushed the santihernandezc/implement_default_email_sender branch from da181cb to 82a492a Compare June 19, 2024 15:22
@santihernandezc santihernandezc force-pushed the santihernandezc/implement_default_email_sender branch from da8d61e to 887ca4d Compare June 19, 2024 16:02
@santihernandezc santihernandezc marked this pull request as ready for review June 19, 2024 16:36
@santihernandezc santihernandezc requested a review from a team as a code owner June 19, 2024 16:36
@santihernandezc santihernandezc changed the title (WIP) Implement default email sender Implement default email sender Jun 19, 2024
Copy link
Contributor

@stevesg stevesg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed this primarily by comparing to the Grafana code which it same from, looks good. I have left a few questions.

I'd like us to raise a follow-up issue about adding back tracing into the sender, though we can do this later on after integration into Mimir.

Comment on lines 93 to 96
var buffer bytes.Buffer
if err := s.tmpl.ExecuteTemplate(&buffer, cmd.Template, data); err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Grafana code here supports providing multiple alternative content types, but we're only doing HTML here. Should we retain text/plan support too?


func (s *defaultEmailSender) setDefaultTemplateData(data map[string]any) {
data["AppUrl"] = s.cfg.ExternalURL
data["BuildVersion"] = s.cfg.Version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have BuildStamp, EmailCodeValidHours and Name here, but they're not used in our template so that makes sense.

}

d := gomail.NewDialer(host, iPort, s.cfg.AuthUser, s.cfg.AuthPassword)
d.TLSConfig = tlsconfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not setting StartTLSPolicy here, should we?

Copy link
Contributor

@stevesg stevesg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

@santihernandezc santihernandezc merged commit 0ddd6d9 into main Jun 21, 2024
3 checks passed
@santihernandezc santihernandezc deleted the santihernandezc/implement_default_email_sender branch June 21, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants