milo: buildbucket builer view

milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retrieve Rietveld/Gerrit CL link from build properties

Caveat: ATM not possible to retrieve got_revision from swarmbucket builds

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422
Committed: https://github.com/luci/luci-go/commit/fcc9be2f55080a6e91fd728205342a744c62b2cf

[nodir, 2016-07-08 21:58:47]

Description was changed from

==========
milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.
The "/builders/" is a part of the path because we may want to support
console and/or waterfall in the future.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retreive CL link from build properties

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422
==========

to

==========
milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.
The "/builders/" is a part of the path because we may want to support
console and/or waterfall in the future.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retrieve CL link from build properties

Caveat: ATM not possible to retrieve got_revision from swarmbucket builds

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422
==========

[nodir, 2016-07-08 21:58:56]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 21:58:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nodir, 2016-07-08 21:59:29]

PTAL

Deployed to https://959-3dc1f53-tainted-nodir-dot-luci-milo.appspot.com
For example:
Infra Presubmit:
https://959-3dc1f53-tainted-nodir-dot-luci-milo.appspot.com/buildbucket/maste...
Infra Presubmit (Swarming):
https://959-3dc1f53-tainted-nodir-dot-luci-milo.appspot.com/buildbucket/maste...

[nodir, 2016-07-08 21:59:53]

Description was changed from

==========
milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.
The "/builders/" is a part of the path because we may want to support
console and/or waterfall in the future.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retrieve CL link from build properties

Caveat: ATM not possible to retrieve got_revision from swarmbucket builds

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422
==========

to

==========
milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.
The "/builders/" is a part of the path because we may want to support
console and/or waterfall in the future.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retrieve Rietveld/Gerrit CL link from build properties

Caveat: ATM not possible to retrieve got_revision from swarmbucket builds

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422
==========

[commit-bot: I haz the power, 2016-07-08 22:00:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 22:00:29]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Precise 32 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Linux%20Preci...)
  Luci-go Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Linux%20Trust...)
  Luci-go Linux Trusty 64 Tester (Swarming) on master.tryserver.infra
(JOB_FAILED, https://luci-milo.appspot.com/swarming/prod/2fe55713ace96610)
  Luci-go Mac Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Mac%20Tester/...)
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)
  Luci-go Presubmit (Swarming) on master.tryserver.infra (JOB_FAILED,
https://luci-milo.appspot.com/swarming/prod/2fe55712a1be3210)

[nodir, 2016-07-08 22:01:21]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 22:01:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 22:03:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 22:03:06]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Precise 32 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Linux%20Preci...)
  Luci-go Linux Precise 32 Tester (Swarming) on master.tryserver.infra
(JOB_FAILED, https://luci-milo.appspot.com/swarming/prod/2fe55965958c2210)
  Luci-go Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Linux%20Trust...)
  Luci-go Linux Trusty 64 Tester (Swarming) on master.tryserver.infra
(JOB_FAILED, https://luci-milo.appspot.com/swarming/prod/2fe55966c5274110)
  Luci-go Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Presubmit/bui...)
  Luci-go Presubmit (Swarming) on master.tryserver.infra (JOB_FAILED,
https://luci-milo.appspot.com/swarming/prod/2fe55965a644dd10)

[nodir, 2016-07-08 22:08:28]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 22:08:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nodir, 2016-07-08 22:14:49]

nodir@chromium.org changed reviewers:
+ estaab@chromium.org

[nodir, 2016-07-08 22:14:50]

forgot estaab

[commit-bot: I haz the power, 2016-07-08 22:36:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 22:36:20]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit (Swarming) on master.tryserver.infra (JOB_FAILED,
https://luci-milo.appspot.com/swarming/prod/2fe55fe2c27e1910)

[hinoka, 2016-07-08 23:26:05]

lgtm + suggestions

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
File appengine/cmd/milo/buildbucket/builder.go (right):

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:25:
"github.com/luci/luci-go/common/logging"
Rest of milo uses.  log "github.com/luci/..../logging"

Not sure why, I think Dan just likes it that way.  Might as well keep it for
consistency.

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:124: case "STARTED":
You can set pending interval here too (might be useful for display next to
builds saying it waiting this long, and took this long to run, etc)

Unless, this data isn't available.

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:128: case "COMPLETED":
No started time?

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:227: if err := getDebugBuilds(c,
bucket, builder, maxCompletedBuilds, result); err != nil {
err := getDebugBuilds(...)
return result, err

And then you don't need the else

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:245: if err != nil {
Suggestion: Add a placeholder build, with status infrafailure, with step text
indicating the failure (or "see logs").

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
File appengine/cmd/milo/buildbucket/common.go (right):

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/common.go:30: // Ignores tags that don't have
colon (we don't have them in practice because
nit: "doesn't have a colon"

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/common.go:79: default:
If you remove the defaults and put the error case outside of the switch, you
only need one

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
File appengine/cmd/milo/buildbucket/html.go (right):

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/html.go:59: numBuilds := 0
Shouldn't this default to something like 25?

Oh you set a default if 0 inside the function.  imo since go doesn't have nil
inputs for native types, it makes more sense to use -1 as "default", or just
always treat maxCompletedBuilds strictly

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/fron...
File appengine/cmd/milo/frontend/milo.go (right):

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/fron...
appengine/cmd/milo/frontend/milo.go:34:
r.GET("/buildbucket/:bucket/builders/:builder", basemw,
settings.Wrap(buildbucket.Builder{}))
Is there anything you expect to see from a bucket other than builders? If not
/buildbucket/:bucket/:builder is cleaner

[nodir, 2016-07-09 00:00:49]

redeployed https://959-3dc1f53-tainted-nodir-dot-luci-milo.appspot.com

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
File appengine/cmd/milo/buildbucket/builder.go (right):

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:25:
"github.com/luci/luci-go/common/logging"
On 2016/07/08 23:26:05, hinoka wrote:
> Rest of milo uses.  log "github.com/luci/..../logging"
> 
> Not sure why, I think Dan just likes it that way.  Might as well keep it for
> consistency.

Done.

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:124: case "STARTED":
On 2016/07/08 23:26:05, hinoka wrote:
> You can set pending interval here too (might be useful for display next to
> builds saying it waiting this long, and took this long to run, etc)
> 
> Unless, this data isn't available.

good point, done

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:128: case "COMPLETED":
On 2016/07/08 23:26:05, hinoka wrote:
> No started time?

unfortunately buildbucket does not provide it; updated comment

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:227: if err := getDebugBuilds(c,
bucket, builder, maxCompletedBuilds, result); err != nil {
On 2016/07/08 23:26:05, hinoka wrote:
> err := getDebugBuilds(...)
> return result, err
> 
> And then you don't need the else

Done.

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/builder.go:245: if err != nil {
On 2016/07/08 23:26:05, hinoka wrote:
> Suggestion: Add a placeholder build, with status infrafailure, with step text
> indicating the failure (or "see logs").

good idea, done

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
File appengine/cmd/milo/buildbucket/common.go (right):

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/common.go:30: // Ignores tags that don't have
colon (we don't have them in practice because
On 2016/07/08 23:26:05, hinoka wrote:
> nit: "doesn't have a colon"

Done.

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/common.go:79: default:
On 2016/07/08 23:26:05, hinoka wrote:
> If you remove the defaults and put the error case outside of the switch, you
> only need one

actually you revealed a bug: the first error should be about build result, not
build status. Fixed that

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
File appengine/cmd/milo/buildbucket/html.go (right):

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbucket/html.go:59: numBuilds := 0
On 2016/07/08 23:26:05, hinoka wrote:
> Shouldn't this default to something like 25?
> 
> Oh you set a default if 0 inside the function.  imo since go doesn't have nil
> inputs for native types, it makes more sense to use -1 as "default", or just
> always treat maxCompletedBuilds strictly

ok I will treat anything <0 as "use default"
-1 is rather standard, e.g. if a rune is found in a string, -1 is returned

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/fron...
File appengine/cmd/milo/frontend/milo.go (right):

https://codereview.chromium.org/2134673003/diff/40001/appengine/cmd/milo/fron...
appengine/cmd/milo/frontend/milo.go:34:
r.GET("/buildbucket/:bucket/builders/:builder", basemw,
settings.Wrap(buildbucket.Builder{}))
On 2016/07/08 23:26:05, hinoka wrote:
> Is there anything you expect to see from a bucket other than builders? If not
> /buildbucket/:bucket/:builder is cleaner

yeah, see comment description

[hinoka, 2016-07-09 00:04:20]

Re: console/waterfall.  The idea with consoles and waterfalls are that they can
be protocol-agnostics.  ie you can define a console that is a mix of buildbucket
and buildbot builders.  That means they'd go under their own top level path
(/console/<name> for instance) and not buildbucket.

[nodir, 2016-07-09 00:06:16]

Patchset #4 (id:60001) has been deleted

[nodir, 2016-07-09 00:06:37]

Description was changed from

==========
milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.
The "/builders/" is a part of the path because we may want to support
console and/or waterfall in the future.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retrieve Rietveld/Gerrit CL link from build properties

Caveat: ATM not possible to retrieve got_revision from swarmbucket builds

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422
==========

to

==========
milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retrieve Rietveld/Gerrit CL link from build properties

Caveat: ATM not possible to retrieve got_revision from swarmbucket builds

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422
==========

[nodir, 2016-07-09 00:07:14]

On 2016/07/09 00:04:20, hinoka wrote:
> Re: console/waterfall.  The idea with consoles and waterfalls are that they
can
> be protocol-agnostics.  ie you can define a console that is a mix of
buildbucket
> and buildbot builders.  That means they'd go under their own top level path
> (/console/<name> for instance) and not buildbucket.

makes sense
removed /builders from path and updated CL decription

[nodir, 2016-07-09 00:07:27]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-07-09 00:07:27]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/2134673003/#ps100001
(title: "remove /builders from path")

[commit-bot: I haz the power, 2016-07-09 00:07:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-09 00:11:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-09 00:11:56]

Try jobs failed on following builders:
  Luci-go Linux Precise 32 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-go%20Linux%20Preci...)
  Luci-go Linux Precise 32 Tester (Swarming) on master.tryserver.infra
(JOB_FAILED, https://luci-milo.appspot.com/swarming/prod/2fe5ccf69f1c4d10)
  Luci-go Linux Trusty 64 Tester (Swarming) on master.tryserver.infra
(JOB_FAILED, https://luci-milo.appspot.com/swarming/prod/2fe5ccf782412810)

[nodir, 2016-07-12 16:13:21]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-07-12 16:13:21]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/2134673003/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2016-07-12 16:13:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 16:28:27]

Description was changed from

==========
milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retrieve Rietveld/Gerrit CL link from build properties

Caveat: ATM not possible to retrieve got_revision from swarmbucket builds

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422
==========

to

==========
milo: buildbucket builer view

add /buildbucket/:bucket/builders/:builder handler that fetches builds
from buildbucket and renders a builder page.

For a each build:
- try to retrieve buildbot build number or swarming task id for a nice
  URL label
- try to retrieve Rietveld/Gerrit CL link from build properties

Caveat: ATM not possible to retrieve got_revision from swarmbucket builds

Also fix some minor bugs.

R=hinoka@chromium.org
BUG=613422

Committed:
https://github.com/luci/luci-go/commit/fcc9be2f55080a6e91fd728205342a744c62b2cf
==========

[commit-bot: I haz the power, 2016-07-12 16:28:28]

Committed patchset #6 (id:120001) as
https://github.com/luci/luci-go/commit/fcc9be2f55080a6e91fd728205342a744c62b2cf

[commit-bot: I haz the power, 2016-07-12 16:28:30]

CQ bit was unchecked.