added email notification text content support #934

awaragi · 2017-07-31T16:52:45Z

added email notification text content support (configuration text)
added default email notification text content default value empty
converted email notification from html to multipart/alternatives with support to both html (first) and text (last)
ignore intellij IDE .idea working folder

added default email notification text content default value empty converted email notification from html to multipart/alternatives with support to both html (first) and text (last) ignore intellij IDE .idea working folder

mxinden

@awaragi I think the convention in the Prometheus organization is to not add editor specific .gitignore entries. Please add them to your global .gitignore instead.

fabxc · 2017-08-02T09:22:03Z

Thanks for picking this up!

Go's standard library actually has a package for multipart: https://godoc.org/mime/multipart
It seems cleaner to use this rather than constructing everything ourselves. Could you switch to using that?

fabxc · 2017-08-02T09:02:53Z

config/notifiers.go

@@ -142,6 +143,7 @@ type EmailConfig struct {
 	AuthIdentity string            `yaml:"auth_identity,omitempty" json:"auth_identity,omitempty"`
 	Headers      map[string]string `yaml:"headers,omitempty" json:"headers,omitempty"`
 	HTML         string            `yaml:"html,omitempty" json:"html,omitempty"`
+	TEXT         string            `yaml:"text,omitempty" json:"text,omitempty"`


Just Text as it's not an acronym.

Hi @fabxc

I am not sure I understand your comment. I am not a Go dev (yet) so I need more details about what you are requesting as a change.

I think the key here is that HTML is all caps since it it an initialism, but TEXT would be written as Text since it's just a word.

Does that make sense?
https://github.com/golang/go/wiki/CodeReviewComments#initialisms gives move info on this case

awaragi · 2017-08-02T13:29:08Z

@fabxc The original code of email notification was constructed manually from smtp handshake to email construction. I am not at the level to rework this entire section of code. The smtp protocole is old dated and is not likely to change any time soon. may I recommend adding //TODO refactor to use standard multipart library https://godoc.org/mime/multipart?

added TODO note to refactor multipart code to use standard go library

awaragi · 2017-08-09T17:51:37Z

Hi, any progress with this pull request? I see that the flag for changes requested is still there but I have removed the offending file (.gitignore).

@mxinden can you please mark your change request as satisfied?

Editor specific gitignore entry removed.

mxinden · 2017-08-09T18:02:06Z

@awaragi For sure. Thanks for the reminder.

@fabxc Can you have a second look at this?

fabxc · 2017-08-10T08:54:14Z

@fabxc The original code of email notification was constructed manually from smtp handshake to email construction. I am not at the level to rework this entire section of code. The smtp protocole is old dated and is not likely to change any time soon. may I recommend adding //TODO refactor to use standard multipart library https://godoc.org/mime/multipart?

Sorry, I've to raise a very clear "NO" on this. Every new feature is a maintenance liability and already having pending TODOs when adding them is not sustainable.
The question is, who is going to refactor this? The answer is typically no one.

I hope my concern is understandable.

I'm happy to review any attempts to use the proper standard library package. If you don't feel like trying it yourself, maybe someone else who's keen on this feature wants to give it a shot.

…l parts and bounderies

awaragi · 2017-08-10T14:03:18Z

@fabxc I understand your point. My liking to Go is increasing so I've attempted to use the mime/multipart library. Take another look and let me know if it works.

By the way, your original TODO about adding support text template is now removed so one less to go.

mxinden · 2017-08-13T15:38:44Z

As far as I can tell (for documentation purposes): Fixes #290

awaragi · 2017-08-14T23:26:16Z

Hey Guys @fabxc @mxinden , any other suggestions, modifications or can you accept this PR?

awaragi · 2017-08-18T14:54:24Z

Any chance someone can take a look at this PR?

fabxc

Thanks for putting in the work! Looks good in general. Just a few remarks on usage of the multipart package and we should be good to go.

fabxc · 2017-08-18T15:12:36Z

notify/impl.go

 	fmt.Fprintf(wc, "Date: %s\r\n", time.Now().Format(time.RFC1123Z))
+	fmt.Fprintf(wc, "Content-Type: multipart/alternative;  boundary=\"%s\"\r\n", multipartWriter.Boundary())


Looks like this can be replaced with this convenience function: https://godoc.org/mime/multipart#Writer.FormDataContentType

Hi Fabian @fabxc, your initial thought is correct however for multipart email content, you need the multipart/alternative instead of multipart/form-data which is what the FormDataContentType return. I did however notice an additional quotes that were problematic that I took out from around the boundary string.

fabxc · 2017-08-18T15:15:14Z

notify/impl.go


 	// TODO: Add some useful headers here, such as URL of the alertmanager
 	// and active/resolved.
 	fmt.Fprintf(wc, "\r\n")

-	// TODO(fabxc): do a multipart write that considers the plain template.
+	// Html template
+	multipartWriter.CreatePart(textproto.MIMEHeader{"Content-Type": {"text/html; charset=UTF-8"}})


According to the documentation of CreatePart the io.Writer it returns should be used to write the output of the template into.
Same below when creating the part for the text template.

https://godoc.org/mime/multipart#Writer.CreatePart

The returned error should be checked as well.

@fabxc implemented as suggested and added basic error handling.

awaragi · 2017-08-18T18:07:08Z

Thanks for the feedback. I will update accordingly.

added error handling while creating parts removed unnecessary quotes from boundry string

stuartnelson3 · 2017-08-22T23:07:13Z

notify/impl.go

 	if err != nil {
 		return true, err
 	}

+	// closing multi-part content


This comment probably isn't necessary, since the code says essentially the same thing.

stuartnelson3 · 2017-08-22T23:07:29Z

notify/impl.go

+	// closing multi-part content
+	multipartWriter.Close()
+
+	// writing multipart content


Same as above comment.

stuartnelson3 · 2017-08-25T07:45:54Z

@fabxc any chance to take another look?

awaragi · 2017-09-01T00:31:33Z

@fabxc any possibility to move forward with this pull request?

awaragi · 2017-09-03T21:30:16Z

Anyone?

awaragi · 2017-09-05T23:50:58Z

Any possibility of merging this pull request?

stuartnelson3 · 2017-09-06T08:04:48Z

Sorry for the delay, I'll try to poke @fabxc again

fabxc · 2017-09-07T15:24:11Z

Thanks, LGTM! Sorry about the long delay.

* added email notification text content support (configuration text) added default email notification text content default value empty converted email notification from html to multipart/alternatives with support to both html (first) and text (last) ignore intellij IDE .idea working folder * removed specific editor .gitignore entries * renamed TEXT to Text as it's not an acronym added TODO note to refactor multipart code to use standard go library * refactored to use standard go mime/multipart library for text and html parts and bounderies * use multipart createPart returned writer added error handling while creating parts removed unnecessary quotes from boundry string * removed unnecessary comments

alin-amana · 2017-09-29T07:32:03Z

This causes a regression.

I've built alertmanager from head and now I'm getting multipart emails with the old html content and empty text content. If your email client chooses the text format (which Gmail does for me, for some reason) then the content of the alert email is empty.

The code should check for non-empty html and/or text templates and skip the empty one, if any.

alin-amana · 2017-09-29T08:03:56Z

Also, a MIME-Version: 1.0 header must be added: http://www.rfc-base.org/txt/rfc-2045.txt

alin-amana · 2017-09-29T08:10:40Z

And, per RFC 2046 section 5.1.4, the parts should be in increasing preference order. Meaning text should come before HTML. This explains why I'm seeing the empty plain text version in Gmail.

Never mind, I'll submit a PR.

stuartnelson3 · 2017-09-29T08:27:22Z

@alin-amana Are you working on this currently? This is a big enough issue that a new release will go out when it's fixed.

alin-amana · 2017-09-29T08:32:37Z

Yup, working as we speak.

alin-amana · 2017-09-29T08:44:39Z

Done, PTAL at #1009.

RichiH · 2017-10-19T13:59:36Z

Huh, missed that one. Thanks to all involved!

* Do not rely on AArch64 CPUs to support 32-bit ARM for cross-testing. Signed-off-by: Alexey Kopytov <akopytov@gmail.com> * aarch64 like ppc64le reports 64k node_sockstat_TCP_mem_bytes due to 64k pages. Signed-off-by: Alexey Kopytov <akopytov@gmail.com>

awaragi mentioned this pull request Jul 31, 2017

Allow text/plain emails #926

Closed

mxinden previously requested changes Aug 1, 2017

View reviewed changes

mxinden added component/notify component/templating kind/enhancement needs review labels Aug 1, 2017

removed specific editor .gitignore entries

3dbe274

fabxc reviewed Aug 2, 2017

View reviewed changes

renamed TEXT to Text as it's not an acronym

54ed71e

added TODO note to refactor multipart code to use standard go library

refactored to use standard go mime/multipart library for text and htm…

3fdfc79

…l parts and bounderies

fabxc suggested changes Aug 18, 2017

View reviewed changes

Pierre Awaragi added 2 commits August 22, 2017 13:26

Merge branch 'master-upstream'

fbd8a54

use multipart createPart returned writer

c5c46f8

added error handling while creating parts removed unnecessary quotes from boundry string

stuartnelson3 reviewed Aug 22, 2017

View reviewed changes

Pierre Awaragi added 2 commits August 23, 2017 14:42

Merge branch 'master-upstream'

a039d6e

removed unnecessary comments

e4bba3b

fabxc merged commit f3cd397 into prometheus:master Sep 7, 2017

alin-amana mentioned this pull request Sep 29, 2017

Fix multipart email implementation #1009

Merged

This was referenced Mar 16, 2018

Document smtp_hello parameter prometheus/docs#988

Merged

headers: Content-Type: doesn't work #542

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added email notification text content support #934

added email notification text content support #934

awaragi commented Jul 31, 2017

mxinden left a comment

fabxc commented Aug 2, 2017

fabxc Aug 2, 2017

awaragi Aug 2, 2017 •

edited

Loading

Kellel Aug 2, 2017

awaragi commented Aug 2, 2017

awaragi commented Aug 9, 2017 •

edited

Loading

mxinden commented Aug 9, 2017

fabxc commented Aug 10, 2017

awaragi commented Aug 10, 2017

mxinden commented Aug 13, 2017

awaragi commented Aug 14, 2017

awaragi commented Aug 18, 2017

fabxc left a comment

fabxc Aug 18, 2017

awaragi Aug 22, 2017

fabxc Aug 18, 2017

awaragi Aug 22, 2017

awaragi commented Aug 18, 2017

stuartnelson3 Aug 22, 2017

stuartnelson3 Aug 22, 2017

stuartnelson3 commented Aug 25, 2017

awaragi commented Sep 1, 2017

awaragi commented Sep 3, 2017

awaragi commented Sep 5, 2017

stuartnelson3 commented Sep 6, 2017

fabxc commented Sep 7, 2017

alin-amana commented Sep 29, 2017

alin-amana commented Sep 29, 2017

alin-amana commented Sep 29, 2017

stuartnelson3 commented Sep 29, 2017

alin-amana commented Sep 29, 2017

alin-amana commented Sep 29, 2017

RichiH commented Oct 19, 2017

		fmt.Fprintf(wc, "Date: %s\r\n", time.Now().Format(time.RFC1123Z))
		fmt.Fprintf(wc, "Content-Type: multipart/alternative; boundary=\"%s\"\r\n", multipartWriter.Boundary())

added email notification text content support #934

added email notification text content support #934

Conversation

awaragi commented Jul 31, 2017

mxinden left a comment

Choose a reason for hiding this comment

fabxc commented Aug 2, 2017

Choose a reason for hiding this comment

awaragi Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaragi commented Aug 2, 2017

awaragi commented Aug 9, 2017 • edited Loading

mxinden commented Aug 9, 2017

fabxc commented Aug 10, 2017

awaragi commented Aug 10, 2017

mxinden commented Aug 13, 2017

awaragi commented Aug 14, 2017

awaragi commented Aug 18, 2017

fabxc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaragi commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartnelson3 commented Aug 25, 2017

awaragi commented Sep 1, 2017

awaragi commented Sep 3, 2017

awaragi commented Sep 5, 2017

stuartnelson3 commented Sep 6, 2017

fabxc commented Sep 7, 2017

alin-amana commented Sep 29, 2017

alin-amana commented Sep 29, 2017

alin-amana commented Sep 29, 2017

stuartnelson3 commented Sep 29, 2017

alin-amana commented Sep 29, 2017

alin-amana commented Sep 29, 2017

RichiH commented Oct 19, 2017

awaragi Aug 2, 2017 •

edited

Loading

awaragi commented Aug 9, 2017 •

edited

Loading