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...@daniel.shahaf.name> > Yasuhito FUTATSUKI <futat...@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.
