Nathan Hartman wrote on Sun, Oct 27, 2019 at 23:14:55 -0400:
> [[[
> Per SVN-1804, any SMTP error terminates mailer.py with an unhandled
> exception. The impact is that emails which would have been sent after
> the exception are silently lost.
*lightbulb*
> Found by: rzigweid
> Review by: Daniel Shahaf <d.s_at_daniel.shahaf.name>
> Yasuhito FUTATSUKI <futatuki_at_poem.co.jp>
Committers may be referred to by their names in COMMITTERS, but non-committers
should be referred to by name, when available.
In this case, having checked <http://subversion.tigris.org/issues/show_bug.cgi?id=1804> to
find rzigweid's name,¹ the conventional form would be:
.
Found by: Robert M. Zigweid
Review by: danielsh
futatuki
Email addresses are optional, in the sense that contribulyzer doesn't require
them. When specified, we tend to escape @ as {_AT_}. (This particular
replacement is recognized by tools/dev/contribulyze.py.)
In my review I specifically noted that I hadn't done a complete review. It would
have been appropriate to add a parenthetical recording that fact.
> +++ tools/hook-scripts/mailer/mailer.py (working copy)
> @@ -285,6 +285,7 @@ class SMTPOutput(MailedOutput):
> self.write(self.mail_headers(group, params))
>
> def finish(self):
> + try:
> if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl
> == 'yes':
Your client inserted a hard line break here. That's not a deal breaker for a
-x-w patch, of course; just a readability thing.
> server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
> else:
> @@ -294,6 +295,9 @@ class SMTPOutput(MailedOutput):
> self.cfg.general.smtp_password)
> server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
> server.quit()
> + except smtplib.SMTPException as detail:
> + sys.stderr.write("mailer.py: SMTP error occurred: %s\n" % (detail,))
> + raise MessageSendFailure
Sorry for not catching this in my previous review, but is there any additional
context we provide to the sysadmin who'll read this error message? For
example, the revision number, repository name, recipients, subject line,
message-id?
The new log message looks good (though the glob pattern did gave me pause), but
I haven't reviewed the logic changes.
Cheers,
Daniel
¹ Aside: When we migrated from tigris to jira, we seem to have lost the "tigris
username to fullname" mapping. I suspect we don't have any backup of it, save for
what archive.org's spider may have cached.
Received on 2019-10-28 09:08:31 CET