Add emailHeaders for extra mail headers #143
Conversation
This optional configuration option, which allows you to add additional headers to the mail
src/slurmmail/cli.py
Outdated
| @@ -891,6 +898,11 @@ def send_mail_main(): | |||
| options.email_from_name = config.get(section, "emailFromName") | |||
| options.email_subject = config.get(section, "emailSubject") | |||
|
|
|||
| if config.has_option(section, "emailHeaders"): | |||
| email_headers = [ x.strip().split(":", maxsplit=1) for x in config.get(section, "emailHeaders").split(";") ] | |||
There was a problem hiding this comment.
To populate the options.email_headers dictionary, two loops are being used here, one to create email_headers and then another to iterate through this list. Instead this can be accomplished with one loop which is slightly easier to read also:
for header in config.get(section, "emailHeaders").split(";"):
header_name, header_value = header.split(":", maxsplit=1)
email_headers_dict[header_name.strip()] = header_value.strip()There was a problem hiding this comment.
Adjusted based on the feedback
src/slurmmail/cli.py
Outdated
| if config.has_option(section, "emailHeaders"): | ||
| email_headers = [ x.strip().split(":", maxsplit=1) for x in config.get(section, "emailHeaders").split(";") ] | ||
| for email_header in email_headers: | ||
| options.email_headers[email_header[0].strip()] = email_header[1].strip() |
There was a problem hiding this comment.
We also need to ensure that the user had not provided e-mail headers that are set by Slurm-Mail. These are:
ToFromDateMessage-ID
Otherwise these will get overwritten. Perhaps an array of protected headers could be added which are then checked against when processing the slurm.conf file?
|
Note: the Pylint errors can be ignored in this PR - I am tracking this issue in #145 |
- Changed the double for loop, to more readable. - Added an extra check to make sure we do not overrule the already set headers. Print a warning if this is the case and ignore that header
src/slurmmail/cli.py
Outdated
| if options.email_headers: | ||
| for header_name, header_value in options.email_headers.items(): | ||
| if header_name in msg: | ||
| logger.warning( |
There was a problem hiding this comment.
This should be logging.warning as Slurm-Mail currently uses the "root" logger rather than its own (for now).
There was a problem hiding this comment.
Also, please change to:
logging.warning("Ignoring header_name '%s' - header has already been set", header_name)
src/slurmmail/cli.py
Outdated
| if options.email_headers: | ||
| for header_name, header_value in options.email_headers.items(): | ||
| if header_name in msg: | ||
| logger.warning( |
There was a problem hiding this comment.
Also, please change to:
logging.warning("Ignoring header_name '%s' - header has already been set", header_name)
|
Thank you @dstam for this PR and for adjusting the code after review. This PR will be included in version 4.22. |
This optional configuration option, which allows you to add additional headers to the mail. We needed to set some extra headers to the mail, but instead of hard coding perhaps add it as a configuration option.