Merge lp://staging/~mwhudson/lava-scheduler/email-on-health-check-failure into lp://staging/lava-scheduler

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

On Wed, 04 Apr 2012 22:35:21 -0000, Zygmunt Krynicki <email address hidden> wrote:
> Review: Approve
>
> 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

I added an XXX.

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

Changed. Although I hate that this is necessary; the json variant
always gives you unicode strings, simplejson sometimes gives you
bytestrings, sometimes unicode strings.

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

Oops, thanks.

> 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.

It's done at submission time.

$ lava-tool submit-job http://mwhudson@127.0.0.1:8000/RPC2/ ~/dispatcher-jobs/notify-test
EXPERIMENTAL - SUBJECT TO CHANGE (See --experimental-notice for more info)
ERROR: <Fault 400: "' micahelgmail.com' is not a valid email address.">

> 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.

Yeah. Hm. We _could_ make it a list of usernames, we get the email
from Launchpad during the log in process (and we can make sure that the
robot users have sensible email addresses). We'd have to add a user per
mailing list we want to email (which probably would not be too bad).

> 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.

I don't understand what you mean here.

I changed the template to just take the job though, which works but
leads to a pretty ugly template.

> + 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.

I could put the description/job name in the subject I guess? Yeah, I'll
do that.

> Perhaps we could link that to testing efforts? Then ...

> On Wed, 04 Apr 2012 22:35:21 -0000, Zygmunt Krynicki
> <email address hidden> wrote:
> > Review: Approve

> > 67 + if not isinstance(address, (str, unicode)):
> >
> > basestr
>
> Changed. Although I hate that this is necessary; the json variant
> always gives you unicode strings, simplejson sometimes gives you
> bytestrings, sometimes unicode strings.

Depending on python-dev being available, yes this sucks a lot.

> $ lava-tool submit-job http://mwhudson@127.0.0.1:8000/RPC2/ ~/dispatcher-jobs
> /notify-test
> EXPERIMENTAL - SUBJECT TO CHANGE (See --experimental-notice for more info)
> ERROR: <Fault 400: "' micahelgmail.com' is not a valid email address.">
>
> > 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.
>
> Yeah. Hm. We _could_ make it a list of usernames, we get the email
> from Launchpad during the log in process (and we can make sure that the
> robot users have sensible email addresses). We'd have to add a user per
> mailing list we want to email (which probably would not be too bad).

Yeah, let's do that

>
> > 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.
>
> I don't understand what you mean here.

Well I mean that bundle.test_runs == [] is a different use case from bundle is None

> I changed the template to just take the job though, which works but
> leads to a pretty ugly template.

Waiting for the updated diff now

149 + if len(description) > 80:
150 + description = '...' + description[-78:]

Why the last 78 characters? Note that will make the full line 81 + newline characters long

112 + 'lava_scheduler_app/job_summary_mail.txt',
113 + {
114 + 'job': self,
115 + })

Maybe you can fit that on one line?

178 +{% for run in job.results_bundle.test_runs.all %} | {{ run.test.test_id|ljust:20 }} | {{ run.get_summary_results.pass|default:0|rjust:6 }} | {{ run.get_summary_results.total|default:0|rjust:6 }} |

On the note that this makes the template uglier. Recent django has variables and other goodies in the templates and I'm sure you can rewrite that to be readable if you want.

On Thu, 05 Apr 2012 00:49:18 -0000, Zygmunt Krynicki <email address hidden> wrote:
> Review: Approve
>
> 149 + if len(description) > 80:
> 150 + description = '...' + description[-78:]
>
> Why the last 78 characters? Note that will make the full line 81 + newline characters long

Just a bit arbitrary really. Can bump up the limit to say 200, which
would allow every description in the database today, but avoid complete
silliness.

> 112 + 'lava_scheduler_app/job_summary_mail.txt',
> 113 + {
> 114 + 'job': self,
> 115 + })
>
> Maybe you can fit that on one line?

Yep.

> 178 +{% for run in job.results_bundle.test_runs.all %} | {{ run.test.test_id|ljust:20 }} | {{ run.get_summary_results.pass|default:0|rjust:6 }} | {{ run.get_summary_results.total|default:0|rjust:6 }} |
>
> On the note that this makes the template uglier. Recent django has
> variables and other goodies in the templates and I'm sure you can
> rewrite that to be readable if you want.

Ah, you mean {% with %} and so on? It helps a bit, but having to have
everything on the one line is a bit painful. I've improved this a bit,
am happy to take any more suggestions :-)

I also need to make sure we get full URLs in the emails, currently
they're just the /scheduler/job/16280 sort of thing.

Cheers,
mwh

On Thu, 05 Apr 2012 01:49:18 -0000, Michael Hudson-Doyle <email address hidden> wrote:
> On Thu, 05 Apr 2012 00:49:18 -0000, Zygmunt Krynicki <email address hidden> wrote:
> > Review: Approve
> >
> > 149 + if len(description) > 80:
> > 150 + description = '...' + description[-78:]
> >
> > Why the last 78 characters? Note that will make the full line 81 + newline characters long
>
> Just a bit arbitrary really. Can bump up the limit to say 200, which
> would allow every description in the database today, but avoid complete
> silliness.

I've done this.

> > 112 + 'lava_scheduler_app/job_summary_mail.txt',
> > 113 + {
> > 114 + 'job': self,
> > 115 + })
> >
> > Maybe you can fit that on one line?
>
> Yep.
>
> > 178 +{% for run in job.results_bundle.test_runs.all %} | {{ run.test.test_id|ljust:20 }} | {{ run.get_summary_results.pass|default:0|rjust:6 }} | {{ run.get_summary_results.total|default:0|rjust:6 }} |
> >
> > On the note that this makes the template uglier. Recent django has
> > variables and other goodies in the templates and I'm sure you can
> > rewrite that to be readable if you want.
>
> Ah, you mean {% with %} and so on? It helps a bit, but having to have
> everything on the one line is a bit painful. I've improved this a bit,
> am happy to take any more suggestions :-)
>
> I also need to make sure we get full URLs in the emails, currently
> they're just the /scheduler/job/16280 sort of thing.

And this.

And I've tested it. Can you take a look at how I construct the URLs,
and if that looks OK, please merge it?

See you on the 16th!

Cheers,
mwh

> 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.
There are so many easier ways to send spam than with a lava test job - that only authorized users can submit. I think the mailing list things is a concern here. If you want to create a job and send results to certain individuals and email lists, the process shouldn't require going through and looking up all their lava account ids and filing a request to have an id created for each mailing list before you can submit your job.

On Thu, 05 Apr 2012 10:06:20 -0000, Paul Larson <email address hidden> wrote:
> > 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.
>
> There are so many easier ways to send spam than with a lava test job -
> that only authorized users can submit. I think the mailing list
> things is a concern here. If you want to create a job and send
> results to certain individuals and email lists, the process shouldn't
> require going through and looking up all their lava account ids and
> filing a request to have an id created for each mailing list before
> you can submit your job.

It's not really the spam issue that made me change this. My thinking
was more like this:

1) Putting who gets notified in the job files is a pretty crappy way of
   doing this because you have to coordinate with whoever is submitting
   the job to get on the list.

2) Anything more sophisticated is going to be something you do in the
   web app.

3) The obvious thing to do would be to be able say "notify me of results
   matching <set of conditions>" somehow.

4) This means that notifications are tied to user accounts.

5) So we may as well start down that line now, to avoid confusion later.

Writing it down like this does make it clear that it doesn't have to be
like this: for example, in step 3) we could offer the option of "send
email to such and such an address" instead of just "email me". For the
reasons you give, I think this would reduce friction. Perhaps I'll go
back to email addresses in the job file then... Will think about this
overnight and merge something tomorrow :-)

Cheers,
mwh