Code review comment for lp:~mwhudson/lava-scheduler/email-on-health-check-failure

45 + sha1 = self.results_link.split('/')[-2]

# XXX: depends on permalink from the other part, I really really really feel we should mandate that the dispatcher, when invoked by the scheduler, is not doing the submission and that we really submit internally from inside the app. Still, XXX: OMG-depends-on-dashboard-urls.py note should be there

67 + if not isinstance(address, (str, unicode)):

basestr

68 + print (address, unicode, isinstance(address, unicode))

leftover print

69 + raise ValueError(msg)

I have not looked ad this in full (and launchpad context does not show the function this is in). I hope this is done on submission time (and this makes the submission rejected) and not anytime later. If this is later then perhaps this should be non-fatal.

I also somewhat feel bad about sending messages to unverified emails. I'd be much happier with an user list here and a way (even future proof, not-done-yet) to get the email out of a person account. Feels like a way to send spam in the long end.

83 + def _generate_summary_mail(self):
84 + bundle = self.results_bundle
85 + test_runs = []
86 + if bundle is not None:
87 + for tr in bundle.test_runs.all():
88 + results = tr.get_summary_results()
89 + test_runs.append({
90 + 'name':tr.test.test_id,
91 + 'passes': results.get('pass', 0),
92 + 'total': results.get('total', 0),
93 + })
94 + return render_to_string(
95 + 'lava_scheduler_app/job_summary_mail.txt',
96 + {
97 + 'job': self,
98 + 'test_runs': test_runs,
99 + })

If we cannot get the bundle then perhaps the message should be "optimized" for that. I guess all you need is to pass bundle to the context and let it run in the template. Then perhaps we don't even need any processing on the python side which would be even better.

+ send_mail(
110 + "LAVA job notification", mail, settings.SERVER_EMAIL, recipients)
111 +

It would be nice to have more informative subjects but I realize we cannot do it right now. Perhaps we could link that to testing efforts? Then a testing effort could define a notification subject and have an explicit list of people interested in stuff happening. Anyway, more of a brainstorm-away-from-ML so don't worry about this.

Overall mostly good, fix print() and this could go in