mail: Fix regression when sending mail without SSL (v2.7) #47019
Conversation
When this module was refactored in ansible#37098 the non-SSL use-case was broken. The main cause is that we have no way to do integration tests for testing SMTP. This is a back-port to v2.7 of ansible#46403
|
Is this also in |
| @@ -255,6 +255,8 @@ def main(): | |||
| if secure == 'always': | |||
| module.fail_json(rc=1, msg='Unable to start an encrypted session to %s:%s: %s' % | |||
| (host, port, to_native(e)), exception=traceback.format_exc()) | |||
| except: | |||
| pass | |||
There was a problem hiding this comment.
I'd do some debug logging in here. Swallowing wholesale exceptions like this is harmful. Let's at least do except Exception as exc: and add log that + I'd like to see an explicit section for that socket error as well.
There was a problem hiding this comment.
The whole purpose here is that it falls back to the second attempt, except if secure == always.
There was a problem hiding this comment.
Yeah, but it's untrackable this way. Also, there's too much nesting in here. It's hard to reason about it. It's better for maintainability to set explicit expectations for what we want to ignore.
There was a problem hiding this comment.
We should never use a bare except in code. except Exception is acceptable when it is a catchall case (like this). bare except catches things like KeyboardInterrupt and SystemExit which we do not want to catch in 99% of circumstances. In the other 1%, we should be explicit about catching those exceptions so that people can tell we know what we're doing. (Changing to except Exception isn't sufficient to block this as a backport, though. I'm going to merge this and the change can be made in devel).
|
|
Oh, why doesn't it include docs fixes? Why is it partial? |
|
Because the regression is in those 2 lines, not the documentation. |
|
Hi @dagwieers, thank you for submitting this pull-request! |
ansibot
added
backport
|
Mmm... before merging, though, this needs a changelog fragment. I can merge after that is added. |
|
Pro tip: @abadger, you can actually do it yourself (via UI) at https://github.com/dagwieers/ansible/new/mail-regression27/changelogs/fragments if that's faster :) |
|
@webknjaz yep, i know that i can fix up prs several ways. But that only helps if i know what should be in the changelog. Much better for the submitter to add the changelog since they know what it should say without asking around |
|
Merged for 2.7.1 |
SUMMARY
When this module was refactored in #37098 the non-SSL use-case was broken.
The main cause is that we have no way to do integration tests for testing SMTP.
This is a back-port to v2.7 of #46403
This fixes #46673
ISSUE TYPE
COMPONENT NAME
mail
ANSIBLE VERSION
v2.7