code review 135520043: coordinator: add support for different build dashboards

coordinator: add support for different build dashboards

[cmang, 2014-09-05 20:03:47]

Hello bradfitz@golang.org (cc: adg@golang.org,
golang-codereviews@googlegroups.com),

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

[bradfitz, 2014-09-05 20:11:41]

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
File dashboard/coordinator/main.go (right):

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:29: maxBuilds     = flag.Int("maxbuilds", 7, "Max
concurrent builds")
don't change this. it's not related to the stuff below. it's a function of the
machine's capacity.  6 is just a safe default.

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:59: type dashboard struct {
this type doesn't seem worth it. it has no methods or other use by itself other
than just being in a buildConfig. let's just add some new fields to buildConfig
instead?

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:85: addBuilder(buildConfig{name: "linux-386",
dash: goDash})
make goDash the default in addBuidler and remove these cahnges.

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:235: func findWork(builderName, url string) (rev
string, err error) {
dashboardURL? dashURL? here and elsewhere.

[cmang, 2014-09-05 20:46:36]

I'm currently testing this for gccgo builds on http://146.148.43.39/.

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
File dashboard/coordinator/main.go (right):

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:29: maxBuilds     = flag.Int("maxbuilds", 7, "Max
concurrent builds")
On 2014/09/05 20:11:40, bradfitz wrote:
> don't change this. it's not related to the stuff below. it's a function of the
> machine's capacity.  6 is just a safe default.

Done.

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:59: type dashboard struct {
On 2014/09/05 20:11:40, bradfitz wrote:
> this type doesn't seem worth it. it has no methods or other use by itself
other
> than just being in a buildConfig. let's just add some new fields to
buildConfig
> instead?

Done.

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:85: addBuilder(buildConfig{name: "linux-386",
dash: goDash})
On 2014/09/05 20:11:40, bradfitz wrote:
> make goDash the default in addBuidler and remove these cahnges.

Done.

https://codereview.appspot.com/135520043/diff/120001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:235: func findWork(builderName, url string) (rev
string, err error) {
On 2014/09/05 20:11:40, bradfitz wrote:
> dashboardURL? dashURL? here and elsewhere.

Done.

[bradfitz, 2014-09-06 20:06:28]

LGTM but see comments

https://codereview.appspot.com/135520043/diff/160001/dashboard/coordinator/ma...
File dashboard/coordinator/main.go (right):

https://codereview.appspot.com/135520043/diff/160001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:282: c.cmdTimeout = 10 * time.Minute
the way addBuilder worked before is it only set things on c if they were already
blank.

https://codereview.appspot.com/135520043/diff/160001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:303: if strings.HasSuffix(c.name, "-gccgo") {
If you do the above and only set empty ones, you can pass these in on line 78
(spilled out over a few lines)

[cmang, 2014-09-09 17:52:25]

https://codereview.appspot.com/135520043/diff/160001/dashboard/coordinator/ma...
File dashboard/coordinator/main.go (right):

https://codereview.appspot.com/135520043/diff/160001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:282: c.cmdTimeout = 10 * time.Minute
On 2014/09/06 20:06:28, bradfitz wrote:
> the way addBuilder worked before is it only set things on c if they were
already
> blank. 

Acknowledged.

https://codereview.appspot.com/135520043/diff/160001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:303: if strings.HasSuffix(c.name, "-gccgo") {
On 2014/09/06 20:06:27, bradfitz wrote:
> If you do the above and only set empty ones, you can pass these in on line 78
> (spilled out over a few lines)

Done.

[cmang, 2014-09-09 17:54:14]

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

coordinator: add support for different build dashboards

LGTM=bradfitz
R=bradfitz
CC=adg, golang-codereviews
https://codereview.appspot.com/135520043

[bradfitz, 2014-09-26 14:47:14]

Chris,

https://codereview.appspot.com/135520043/diff/200001/dashboard/coordinator/ma...
File dashboard/coordinator/main.go (right):

https://codereview.appspot.com/135520043/diff/200001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:277: "-buildroot=/"+conf.tool,
what was the point of this line? This broke the "hg clone" avoidance
optimization, where the docker images already have a fairly-recent hg clone in
them (at "/" + "goroot"), so now instead of just "hg pull" it has to do an "hg
clone" too.

[cmang, 2014-09-26 17:09:39]

https://codereview.appspot.com/135520043/diff/200001/dashboard/coordinator/ma...
File dashboard/coordinator/main.go (right):

https://codereview.appspot.com/135520043/diff/200001/dashboard/coordinator/ma...
dashboard/coordinator/main.go:277: "-buildroot=/"+conf.tool,
On 2014/09/26 14:47:14, bradfitz wrote:
> what was the point of this line? This broke the "hg clone" avoidance
> optimization, where the docker images already have a fairly-recent hg clone in
> them (at "/" + "goroot"), so now instead of just "hg pull" it has to do an "hg
> clone" too.

I added this because I thought the coordinator might run a go and gccgo build on
the same docker image.  This would break the builder as it would use the goroot
respository for the wrong tool.  This isn't a problem with the way coordinator
initiates builds, though.