code review 136540044: dashboard: more robust commit handler, and allow modifi...

dashboard: more robust commit handler, and allow modifications

[bradfitz, 2014-09-14 18:32:45]

Hello golang-codereviews@googlegroups.com (cc: adg@golang.org,
dvyukov@google.com, rsc@golang.org),

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

[rsc, 2014-09-14 20:10:15]

LGTM

maybe it's okay to reenable the build mails

[adg, 2014-09-15 00:29:56]

LGTM I guess

https://codereview.appspot.com/136540044/diff/40001/dashboard/builder/vcs.go
File dashboard/builder/vcs.go (right):

https://codereview.appspot.com/136540044/diff/40001/dashboard/builder/vcs.go#...
dashboard/builder/vcs.go:213: <ChangeNum>{rev}</ChangeNum>
I thought this value was essentially useless, as different checkouts can provide
different numbers.

[bradfitz, 2014-09-15 00:32:02]

In theory, but for a builder with a fresh hg clone checkout and no local
commits (not that we've ever used hg that way anyway), I think ChangeNum is
predictable.


On Sun, Sep 14, 2014 at 8:29 PM, <adg@golang.org> wrote:

> LGTM I guess
>
>
> https://codereview.appspot.com/136540044/diff/40001/
> dashboard/builder/vcs.go
> File dashboard/builder/vcs.go (right):
>
> https://codereview.appspot.com/136540044/diff/40001/
> dashboard/builder/vcs.go#newcode213
> dashboard/builder/vcs.go:213: <ChangeNum>{rev}</ChangeNum>
> I thought this value was essentially useless, as different checkouts can
> provide different numbers.
>
> https://codereview.appspot.com/136540044/
>

[adg, 2014-09-15 00:41:33]

If you do a fresh checkout every time, maybe. But I think it also depends
on the order in which the hg server sends you the changes.

As soon as you do "hg pull" all bets are off, though.

[adg, 2014-09-18 00:00:50]

https://codereview.appspot.com/136540044/diff/40001/dashboard/app/build/handl...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/136540044/diff/40001/dashboard/app/build/handl...
dashboard/app/build/handler.go:150: if com.ParentHash != "" {
I think the original test is fine. This test is so that we don't look for a
previous commit if this is the first commit we have seen. We don't actually want
the dashboard to build from the very first commit.

https://codereview.appspot.com/136540044/diff/40001/dashboard/app/build/handl...
dashboard/app/build/handler.go:163: if p.Path == "" &&
!strings.HasPrefix(com.Desc, "[release-branch") && !isUpdate {
we should probably change the prefix test to just "[" so that it catches the new
branches too

https://codereview.appspot.com/136540044/diff/40001/dashboard/app/build/notif...
File dashboard/app/build/notify.go (right):

https://codereview.appspot.com/136540044/diff/40001/dashboard/app/build/notif...
dashboard/app/build/notify.go:53: // Temporarily always:
i think we can revert all this

https://codereview.appspot.com/136540044/diff/40001/dashboard/builder/http.go
File dashboard/builder/http.go (right):

https://codereview.appspot.com/136540044/diff/40001/dashboard/builder/http.go...
dashboard/builder/http.go:182: "Num":               l.ChangeNum,
no no no, revert

https://codereview.appspot.com/136540044/diff/40001/dashboard/builder/main.go
File dashboard/builder/main.go (right):

https://codereview.appspot.com/136540044/diff/40001/dashboard/builder/main.go...
dashboard/builder/main.go:64: commitOnly     = flag.Bool("commitWatchOnly",
false, "only run the commit watch loop and don't do any builds")
this is useful

https://codereview.appspot.com/136540044/diff/40001/dashboard/builder/vcs.go
File dashboard/builder/vcs.go (right):

https://codereview.appspot.com/136540044/diff/40001/dashboard/builder/vcs.go#...
dashboard/builder/vcs.go:193: ChangeNum int // linear change num
revert this

[adg, 2014-09-22 04:10:28]

Ping.

[bradfitz, 2014-09-22 14:30:26]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, adg@golang.org (cc:
dvyukov@google.com, golang-codereviews@googlegroups.com),

Please take another look.

[adg, 2014-09-23 00:30:10]

LGTM

https://codereview.appspot.com/136540044/diff/60001/dashboard/builder/vcs.go
File dashboard/builder/vcs.go (right):

https://codereview.appspot.com/136540044/diff/60001/dashboard/builder/vcs.go#...
dashboard/builder/vcs.go:205: <Parent>{p1node}</Parent>
split this change into a separate cl?

[bradfitz, 2014-09-23 18:30:52]

https://codereview.appspot.com/136540044/diff/40001/dashboard/app/build/handl...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/136540044/diff/40001/dashboard/app/build/handl...
dashboard/app/build/handler.go:150: if com.ParentHash != "" {
On 2014/09/18 00:00:49, adg wrote:
> I think the original test is fine. This test is so that we don't look for a
> previous commit if this is the first commit we have seen. We don't actually
want
> the dashboard to build from the very first commit.

I disagree. This line was giving too much meaning to the "Num" field. The
comment says it's about testing the parent, but then it goes to use the sort
order.

https://codereview.appspot.com/136540044/diff/40001/dashboard/app/build/handl...
dashboard/app/build/handler.go:163: if p.Path == "" &&
!strings.HasPrefix(com.Desc, "[release-branch") && !isUpdate {
On 2014/09/18 00:00:49, adg wrote:
> we should probably change the prefix test to just "[" so that it catches the
new
> branches too

Done.

[bradfitz, 2014-09-23 18:30:55]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, adg@golang.org (cc:
dvyukov@google.com, golang-codereviews@googlegroups.com),

Please take another look.

[adg, 2014-09-26 01:03:59]

LGTM

[bradfitz, 2014-09-26 01:06:21]

*** Submitted as
https://code.google.com/p/go/source/detail?r=1d7d1b189c3e&repo=tools ***

dashboard: more robust commit handler, and allow modifications

LGTM=rsc, adg
R=golang-codereviews, rsc, adg
CC=dvyukov, golang-codereviews
https://codereview.appspot.com/136540044