code review 5490081: dashboard: send mail on build failure

dashboard: send mail on build failure

[adg, 2011-12-20 06:53:46]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[adg, 2011-12-20 21:12:15]

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

Please take another look.

[rsc, 2011-12-20 21:35:41]

The App Engine stuff is hard to follow without comments.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/buil...
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:212: func (l *Log) String() string {
This is pretty heavy-weight for a string method.
I would expect this to be called Text or something
and let String just return string(l.CompressedLog).
If you put l in a Print statement you will be sorry.
Also there's no reason to return a string; return []byte
and save an allocation + copy.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:217: var buf bytes.Buffer
use ioutil.ReadAll

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:530: fmt.Fprint(w, l)
w.Write(l.Text())

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/fail...
File misc/dashboard/app/build/fail-notify.txt (right):

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/fail...
misc/dashboard/app/build/fail-notify.txt:9: {{tail 100 .Log.String}}
.Log.Text | printf "%s" | tail 100
or change signature of tail

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/noti...
File misc/dashboard/app/build/notify.go (right):

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/noti...
misc/dashboard/app/build/notify.go:32: // have been retrieved from the datastore
within that transaction.
Seems like this comment should say something about
the magic of the delay, which is never explained.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/noti...
misc/dashboard/app/build/notify.go:86: func getFirst(c appengine.Context, q
*datastore.Query, v interface{}) os.Error {
http://golang.org/doc/effective_go.html#commentary
http://golang.org/doc/effective_go.html#Getters

This can be firstFoo for whatever Foo it is the first of.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/noti...
misc/dashboard/app/build/notify.go:103: func sendFailMail(c appengine.Context,
com *Commit, builder string) {
http://golang.org/doc/effective_go.html#commentary

[adg1, 2011-12-21 01:06:48]

PTAL

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/buil...
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:212: func (l *Log) String() string {
On 2011/12/20 21:35:41, rsc wrote:
> This is pretty heavy-weight for a string method.
> I would expect this to be called Text or something
> and let String just return string(l.CompressedLog).
> If you put l in a Print statement you will be sorry.
> Also there's no reason to return a string; return []byte
> and save an allocation + copy.

Done.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:217: var buf bytes.Buffer
On 2011/12/20 21:35:41, rsc wrote:
> use ioutil.ReadAll

Done.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:530: fmt.Fprint(w, l)
On 2011/12/20 21:35:41, rsc wrote:
> w.Write(l.Text())

Done.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/fail...
File misc/dashboard/app/build/fail-notify.txt (right):

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/fail...
misc/dashboard/app/build/fail-notify.txt:9: {{tail 100 .Log.String}}
On 2011/12/20 21:35:41, rsc wrote:
> .Log.Text | printf "%s" | tail 100
> or change signature of tail

Done.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/noti...
File misc/dashboard/app/build/notify.go (right):

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/noti...
misc/dashboard/app/build/notify.go:32: // have been retrieved from the datastore
within that transaction.
On 2011/12/20 21:35:41, rsc wrote:
> Seems like this comment should say something about
> the magic of the delay, which is never explained.

Done.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/noti...
misc/dashboard/app/build/notify.go:86: func getFirst(c appengine.Context, q
*datastore.Query, v interface{}) os.Error {
On 2011/12/20 21:35:41, rsc wrote:
> http://golang.org/doc/effective_go.html#commentary
> http://golang.org/doc/effective_go.html#Getters
> 
> This can be firstFoo for whatever Foo it is the first of.

Done.

Again, I used "get" because this effectively does a datastore.Get for a query.

http://codereview.appspot.com/5490081/diff/6001/misc/dashboard/app/build/noti...
misc/dashboard/app/build/notify.go:103: func sendFailMail(c appengine.Context,
com *Commit, builder string) {
On 2011/12/20 21:35:41, rsc wrote:
> http://golang.org/doc/effective_go.html#commentary

Done.

[rsc, 2011-12-21 02:00:19]

LGTM

Much nicer.

http://codereview.appspot.com/5490081/diff/7009/misc/dashboard/app/build/noti...
File misc/dashboard/app/build/notify.go (right):

http://codereview.appspot.com/5490081/diff/7009/misc/dashboard/app/build/noti...
misc/dashboard/app/build/notify.go:85: // firstEntity executes the query q and
loads the first entity into v.
so that's what it does.

firstMatch?

[adg, 2011-12-21 02:16:52]

*** Submitted as f82e8102025c ***

dashboard: send mail on build failure

R=rsc, adg
CC=golang-dev
http://codereview.appspot.com/5490081