code review 4178048: misc/dashboard: notify golang-dev on build failure

misc/dashboard: notify golang-dev on build failure

Fixes issue 1229.

[adg, 2011-02-14 02:37:30]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc1, 2011-02-14 02:55:52]

happy to see this

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/const.py
File misc/dashboard/godashboard/const.py (right):

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/const...
misc/dashboard/godashboard/const.py:11: mail_fail_subject  = "\"%s\" broke %s"
you limit the \"%s\" length so that it doesn't get lost,
but the limit can be removed entirely if you reverse
the arguments:

mail_fail_subject = "%s broken by %s"

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/fail-...
File misc/dashboard/godashboard/fail-notify.txt (right):

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/fail-...
misc/dashboard/godashboard/fail-notify.txt:1: A change authored by {{user}}
broke the {{builder}} build.
Cut through the boilerplate:

---
Change {{node}} broke the {{builder}} build:
http://godashboard.appspot.com/log/{{loghash}}

{{desc}}

http://code.google.com/p/go/source/detail?r={{node}}
---

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/gobui...
File misc/dashboard/godashboard/gobuild.py (right):

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/gobui...
misc/dashboard/godashboard/gobuild.py:292: user = self.request.get('user')
if desc needs encode('utf8') presumably user does too

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/gobui...
misc/dashboard/godashboard/gobuild.py:303: p = q.get()
s/p/parent/ 
now that it's being used across a 50 line span

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/gobui...
misc/dashboard/godashboard/gobuild.py:368: db.run_in_transaction(mark_sent)
I don't know what run_in_transaction means but it seems
a little late: the mail has already been sent.  Why is this
different from all the other writes and not just

n.fail_notification_sent = True
n.put()

[adg, 2011-02-14 19:04:18]

PTAL

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/const.py
File misc/dashboard/godashboard/const.py (right):

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/const...
misc/dashboard/godashboard/const.py:11: mail_fail_subject  = "\"%s\" broke %s"
On 2011/02/14 02:55:53, rsc1 wrote:
> you limit the \"%s\" length so that it doesn't get lost,
> but the limit can be removed entirely if you reverse
> the arguments:
> 
> mail_fail_subject = "%s broken by %s"

Done.

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/fail-...
File misc/dashboard/godashboard/fail-notify.txt (right):

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/fail-...
misc/dashboard/godashboard/fail-notify.txt:1: A change authored by {{user}}
broke the {{builder}} build.
On 2011/02/14 02:55:53, rsc1 wrote:
> Cut through the boilerplate:
> 
> ---
> Change {{node}} broke the {{builder}} build:
> http://godashboard.appspot.com/log/%7B%7Bloghash%7D%7D
> 
> {{desc}}
> 
> http://code.google.com/p/go/source/detail?r=%7B%7Bnode%7D%7D
> ---

Done.

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/gobui...
File misc/dashboard/godashboard/gobuild.py (right):

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/gobui...
misc/dashboard/godashboard/gobuild.py:292: user = self.request.get('user')
On 2011/02/14 02:55:53, rsc1 wrote:
> if desc needs encode('utf8') presumably user does too

Done.

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/gobui...
misc/dashboard/godashboard/gobuild.py:303: p = q.get()
On 2011/02/14 02:55:53, rsc1 wrote:
> s/p/parent/ 
> now that it's being used across a 50 line span

Done.

http://codereview.appspot.com/4178048/diff/1/misc/dashboard/godashboard/gobui...
misc/dashboard/godashboard/gobuild.py:368: db.run_in_transaction(mark_sent)
On 2011/02/14 02:55:53, rsc1 wrote:
> I don't know what run_in_transaction means but it seems
> a little late: the mail has already been sent.  Why is this
> different from all the other writes and not just
> 
> n.fail_notification_sent = True
> n.put()

The transaction is not to prevent multiple mails being sent. It's to prevent
multiple builders simultaneously reading the data, updating it, and clobbering
each other's changes when they write.

run_in_transaction is used earlier in this very function for the same reason
(line 332).

I thought about putting the mail send in the transaction, but the way they are
implemented is that they'll retry if there's a failure. It'd be annoying to see
multiple emails just because the appengine datastore was being flaky.
Conversely, getting mails for different builders that happen to report at
exactly the same time is no big deal.  

http://code.google.com/appengine/docs/python/datastore/transactions.html

[rsc, 2011-02-14 19:41:42]

LGTM

[adg, 2011-02-17 16:34:30]

*** Submitted as http://code.google.com/p/go/source/detail?r=92ec4c5180af ***

misc/dashboard: notify golang-dev on build failure

Fixes issue 1229.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4178048

Committer: Andrew Gerrand <adg@golang.org>