Moves e-mail config into luigi.Config classes #1871
Conversation
|
@daveFNbuck, thanks for your PR! By analyzing the annotation information on this pull request, we identified @jslovan, @jackdwyer and @interskh to be potential reviewers |
|
Yes, yes, yes! :) There still seems to be some valid travis failures. Can you look into it? |
|
Yeah, it broke my pipeline over the weekend too, I'll be fixing it first On Sun, Oct 2, 2016 at 7:23 PM, Arash Rouhani notifications@github.com
|
There was a problem hiding this comment.
Sorry for requesting a minor change this late. But can you update this section? https://github.com/Houzz/luigi/blob/e12093aac2924595185b038b737e0181e8cc9f56/luigi/notifications.py#L24-L29?
Basically the TODO is not fixed, and there's not error-email anymore. :)
kudos on the good name replacement "receiver"! 👍
|
Updated. Glad you like the new name :) |
| config_path=dict(section='core', name='email-type'), | ||
| choices=('plain', 'html', 'none'), | ||
| description='Format type for sent e-mails') | ||
| method = luigi.parameter.ChoiceParameter( |
There was a problem hiding this comment.
A fantastic use of @brcopeland's ChoiceParameter from #1800!
|
This is the kind of patch that make a maintainers day! Great job! :) |
|
Glad you like it :) On Tue, Oct 4, 2016 at 10:00 PM, Arash Rouhani notifications@github.com
|
Description
Moves all configuration access in notifications.py to use luigi.Config classes.
Also sets a default 10 second timeout for smtp.
Motivation and Context
Today when I was trying to figure out why my luigi server stopped responding at 5am, my only clue was that the last line in the log had it sending an e-mail. When I went to the notifications code to see if e-mail has a timeout, I found the config fetching code a bit outdated and hard to read. So I decided to move it to a luigi.Config class like we use in worker and scheduler. This ended up cascading until I had moved all the config accesses into Config classes. As part of this, the parameters are no longer under
[core]but all current settings are still backward-compatible.Since I had started this due to a nasty result of not having an smtp timeout, I gave timeout a 10-second default while I was at it.
Have you tested this? If so, how?
I left the unit tests mostly as-is and they remained passing (except for the ones that don't pass on my machine in master). I only adjusted for changed function signatures in the send_email functions.
I deployed my server with unchanged configuration and triggered a disable e-mail and got it.
I ran the docs test and opened the configuration docs to verify they look right.