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

Add support for plain text only emails #6485

Merged
merged 2 commits into from Jul 22, 2019

Conversation

@leo-lb
Copy link
Contributor

leo-lb commented Jul 17, 2019

Summary

Fixes #6458

Component Name

Email alarms notifications

Additional Information

To enable the functionality add EMAIL_PLAINTEXT_ONLY="YES" in health_alarm_notify.conf

@leo-lb leo-lb requested review from cakrit, Ferroin and thiagoftsm as code owners Jul 17, 2019
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jul 17, 2019

CLA assistant check
All committers have signed the CLA.

@leo-lb

This comment has been minimized.

Copy link
Contributor Author

leo-lb commented Jul 18, 2019

@cakrit The CLA is quite offensive, I don't want to allow my code to be re-used in proprietary software. This is a quite small change that in my opinion is not worth anything, but I certainly wont contribute anything more significant if the CLA does not guarantee me that the code will stay under FOSS licenses. Preferably it should be licensed under the AGPL.

@cakrit

This comment has been minimized.

Copy link
Contributor

cakrit commented Jul 18, 2019

@cakrit The CLA is quite offensive, I don't want to allow my code to be re-used in proprietary software. This is a quite small change that in my opinion is not worth anything, but I certainly wont contribute anything more significant if the CLA does not guarantee me that the code will stay under FOSS licenses. Preferably it should be licensed under the AGPL.

I see how it could be misunderstood. The netdata agent is and forever will be FOSS. Discussing with @ktsaou the wording we have there, we definitely do not intend to ever make it proprietary!!!

Copy link
Contributor

cakrit left a comment

See the comment on the code for the condition (== -> !=)

Copy link
Collaborator

Ferroin left a comment

The current implementation results in the plain-text version of the email existing in the script twice, which in turn makes it a bit more complicated to add any changes to the email format.

Ideally, I think this should build up the email message in a variable, and then pass the contents of that to the send_email function (quick bash scripting tip, you can get the contents of a here-document into a variable using read -r -d '' VAR as the command the here-document is piped into).

@thiagoftsm

This comment has been minimized.

Copy link
Contributor

thiagoftsm commented Jul 19, 2019

Hi @leo-lb ,

For you understand better what @cakrit is saying, let me show the results that I am receiving from your PR. Without to set any new variable in my health_alarm_notify.conf, I received the following e-mail body:

"--multipart-boundary
Content-Type: text/plain; encoding=ANSI_X3.4-1968
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

ceres is critical

ram in use = 51.8% system RAM used"

On the other hand, when I set the variable that you created "EMAIL_PLAINTEXT_ONLY" I received the following e-mail body:

"Evaluated Expression : $this > 5
Expression Variables : [ $this = 52.7362019 ]

The host has 0 WARNING and 1 CRITICAL alarm(s) raised.

--multipart-boundary
Content-Type: text/html; encoding=ANSI_X3.4-1968
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

I am cutting this last e-mail, because it is really big. So as you can see your comparison is delivering the invert result.

Best regards!

@cakrit

This comment has been minimized.

Copy link
Contributor

cakrit commented Jul 19, 2019

Regarding the CLA, we'll add a sentence in the note, stating that we intend to have the netdata agent be forever FOSS.

Another necessary clarification is that the contribution is being made under GPLv3. So even if we ever changed to a different license, anyone could fork the entire repo on the day of the license change and continue on that fork.

The main reason for the wording is that copyright transfer can't be conditional. A contributor either transfers the right or doesn't.

@leo-lb leo-lb force-pushed the leo-lb:leo-lb-plain-text-only-emails branch from 70636dd to 100ef43 Jul 20, 2019
@leo-lb

This comment has been minimized.

Copy link
Contributor Author

leo-lb commented Jul 20, 2019

Sorry for the mistakes. They're all corrected now.

@cakrit I understand, and do hope that keeping this project forever FOSS is the intention. As you have made it now, the CLA is conditionnal I believe? As in, you can't change to a non-FOSS license without breaking the CLA. Thank you for trying to adapt legalese towards that goal.

@leo-lb leo-lb force-pushed the leo-lb:leo-lb-plain-text-only-emails branch from 100ef43 to 3c7196d Jul 20, 2019
@thiagoftsm

This comment has been minimized.

Copy link
Contributor

thiagoftsm commented Jul 20, 2019

Hi @leo-lb ,

You do not need to worry with the fact you do mistakes, because everybody does this, you would need to worry case you ignore them instead to fix, and this is not the case, because I tested your PR now and it worked as we were expecting ! Congratulations!
Before I approve your PR I will only ask more one thing, as you can see in your script, you are defining the variables 'email_plaintext_part' and 'email_html_part' independent of the user choice, so my last suggestion for this PR is you define the variables only case they are really necessary. I think this will be my last request, because I am according with the rest of your implementation.

Best Regards!

@leo-lb

This comment has been minimized.

Copy link
Contributor Author

leo-lb commented Jul 21, 2019

@thiagoftsm The PR was very much half assed and that is why I apologized. These variables were suggested by @Ferroin and they do increase readability because else I would have to write the same content twice, creating code bloat. So I will leave it like that. If you want to make text and html parts configurable, this would be a whole new feature in itself because there's a templating format with macros to create, to embed variables.

@thiagoftsm

This comment has been minimized.

Copy link
Contributor

thiagoftsm commented Jul 21, 2019

@thiagoftsm The PR was very much half assed and that is why I apologized. These variables were suggested by @Ferroin and they do increase readability because else I would have to write the same content twice, creating code bloat. So I will leave it like that. If you want to make text and html parts configurable, this would be a whole new feature in itself because there's a templating format with macros to create, to embed variables.

All right, in my opinion they could be declared inside the respective conditional that it will be used, but considering that the performance gain will be very small, I will approve it, because the main goal was achieved in my tests. More one time, congratulation by your work!

Copy link
Contributor

thiagoftsm left a comment

In all tests that I realized, I got the expected result now. When I set the newest variable in my health_alarm_notify.conf I got the following e-mail body:

Evaluated Expression : $this > 5
Expression Variables : [ $this = 43.7343991 ]

In the following test, when I commented the variable, I got :

$this > 5 Evaluated Expression [ $this = 42.7409247 ] Expression Variables

As it is expected!
Thanks a log @leo-lb , by this work!

@leo-lb

This comment has been minimized.

Copy link
Contributor Author

leo-lb commented Jul 21, 2019

@thiagoftsm I don't think you should worry about variable initialization performance when using Bash. Glad to help. When can this be merged?

@thiagoftsm

This comment has been minimized.

Copy link
Contributor

thiagoftsm commented Jul 21, 2019

Hi @leo-lb ,

I always try to save computer processing time, this is the motive I asked you that changes, but as I said it is not a real problem.
Today is Sunday yet for the majority of the team, so I think other coworkers will check your script during the week and we will merge it!

@leo-lb leo-lb force-pushed the leo-lb:leo-lb-plain-text-only-emails branch from 3c7196d to c5b05ad Jul 21, 2019
@leo-lb

This comment has been minimized.

Copy link
Contributor Author

leo-lb commented Jul 21, 2019

@thiagoftsm Moved the variable initialization around.

Copy link
Contributor

thiagoftsm left a comment

Hi @leo-lb ,

I tested your last changes and they are working fine again! :)
I saw that you moved the HTML inside the "else" and this removed the HTML variable when it is not been used, something that I consider very good, because the computer works with a lot of bytes to mount that message.
You did not move the plain text inside the if, but this is minor as we already talked.

@cakrit
cakrit approved these changes Jul 22, 2019
@cakrit

This comment has been minimized.

Copy link
Contributor

cakrit commented Jul 22, 2019

Sorry for the mistakes. They're all corrected now.

@cakrit I understand, and do hope that keeping this project forever FOSS is the intention. As you have made it now, the CLA is conditionnal I believe? As in, you can't change to a non-FOSS license without breaking the CLA. Thank you for trying to adapt legalese towards that goal.

We can't make copyright transfer conditional, as mentioned in the last sentence of #6485 (comment), but we can't envision any scenarios that would make us move away from FOSS for the agent. It just makes no sense, given the strategy explained in our netdata cloud announcement.

We want the netdata agent to be installed everywhere and the only way to guarantee that is via FOSS.

@cakrit cakrit merged commit aaf3129 into netdata:master Jul 22, 2019
9 of 12 checks passed
9 of 12 checks passed
Header rules - netdata No header rules processed
Details
Pages changed - netdata 2 new files uploaded
Details
Redirect rules - netdata No redirect rules processed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
Mixed content - netdata No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
netlify/netdata/deploy-preview Deploy preview ready!
Details
@leo-lb

This comment has been minimized.

Copy link
Contributor Author

leo-lb commented Jul 22, 2019

Hi @leo-lb ,

I tested your last changes and they are working fine again! :)
I saw that you moved the HTML inside the "else" and this removed the HTML variable when it is not been used, something that I consider very good, because the computer works with a lot of bytes to mount that message.
You did not move the plain text inside the if, but this is minor as we already talked.

The plain text version is always required, it will have to be initialized in ALL cases.

@thiagoftsm

This comment has been minimized.

Copy link
Contributor

thiagoftsm commented Jul 22, 2019

All right, we already merged it !
Thanks !

jacekkolasa added a commit to jacekkolasa/netdata that referenced this pull request Aug 6, 2019
* Add configuration for plain text only emails

* Implement sending Plain Text only emails
jacekkolasa added a commit to jacekkolasa/netdata that referenced this pull request Aug 6, 2019
* Add configuration for plain text only emails

* Implement sending Plain Text only emails
sunflowerbofh added a commit to sunflowerbofh/netdata that referenced this pull request Aug 15, 2019
* Add configuration for plain text only emails

* Implement sending Plain Text only emails
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.