Milo: Extract the rietveld information from buildbot.

Milo: Extract the rietveld information from buildbot.

So that the CL link can bubble up.

BUG=623372
Committed: https://github.com/luci/luci-go/commit/a547622d6472fbe5976d8c030936355464b1316f

[Ryan Tseng, 2016-06-28 00:17:25]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 00:17:34]

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

[hinoka, 2016-06-28 00:18:04]

hinoka@chromium.org changed reviewers:
+ hinoka@chromium.org, nodir@chromium.org

[hinoka, 2016-06-28 00:18:04]

ptal.  Just for rietveld and buildbot, gerrit/buildbot and rietveld/swarmbucket
are next.

[commit-bot: I haz the power, 2016-06-28 00:32:44]

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

[commit-bot: I haz the power, 2016-06-28 00:32:45]

Dry run: This issue passed the CQ dry run.

[nodir, 2016-06-28 02:06:44]

https://codereview.chromium.org/2102903002/diff/1/appengine/cmd/milo/buildbot...
File appengine/cmd/milo/buildbot/build.go (right):

https://codereview.chromium.org/2102903002/diff/1/appengine/cmd/milo/buildbot...
appengine/cmd/milo/buildbot/build.go:362: }
This code will panic if user input malformed, we should not panic if user gave
us bad data. Please add checks in type assertions and log erros if type
assertions fail

https://codereview.chromium.org/2102903002/diff/1/appengine/cmd/milo/buildbot...
appengine/cmd/milo/buildbot/build.go:365: rietveld =
"https://codereview.chromium.org"
Do we have to define defaults in our code? Do we have builds that do not specify
rietveld hostname?

https://codereview.chromium.org/2102903002/diff/1/appengine/cmd/milo/buildbot...
appengine/cmd/milo/buildbot/build.go:370: URL:   fmt.Sprintf("%s/%d", rietveld,
issue),
What if rietveld ends with /
Trim it

[Ryan Tseng, 2016-06-28 18:01:46]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[Ryan Tseng, 2016-06-28 18:01:47]

Review

[commit-bot: I haz the power, 2016-06-28 18:01:57]

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-06-28 18:12:06]

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

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

Dry run: This issue passed the CQ dry run.

[nodir, 2016-06-28 21:44:24]

lgtm

https://codereview.chromium.org/2102903002/diff/20001/appengine/cmd/milo/buil...
File appengine/cmd/milo/buildbot/build.go (right):

https://codereview.chromium.org/2102903002/diff/20001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbot/build.go:352: key := prop[0].(string)
please add a comment to buildbotBuild.Properties that each propetry is a slice
of [propertyName, value, source] where source is optional and where propertyName
and source (if present) are always a string.

otherwise it looks like this will panic too

in general I wish there was a struct buildbotProperty that implements
json.Unmarshaler https://godoc.org/encoding/json#Unmarshaler
this way our code would remain type-safe

[Ryan Tseng, 2016-06-28 21:48:33]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[Ryan Tseng, 2016-06-28 21:48:35]

comment

[Ryan Tseng, 2016-06-28 21:49:31]

https://codereview.chromium.org/2102903002/diff/1/appengine/cmd/milo/buildbot...
File appengine/cmd/milo/buildbot/build.go (right):

https://codereview.chromium.org/2102903002/diff/1/appengine/cmd/milo/buildbot...
appengine/cmd/milo/buildbot/build.go:362: }
On 2016/06/28 02:06:44, nodir wrote:
> This code will panic if user input malformed, we should not panic if user gave
> us bad data. Please add checks in type assertions and log erros if type
> assertions fail

Done.

https://codereview.chromium.org/2102903002/diff/1/appengine/cmd/milo/buildbot...
appengine/cmd/milo/buildbot/build.go:365: rietveld =
"https://codereview.chromium.org"
On 2016/06/28 02:06:44, nodir wrote:
> Do we have to define defaults in our code? Do we have builds that do not
specify
> rietveld hostname?

Not sure, this is just incase.  I'll remove this

https://codereview.chromium.org/2102903002/diff/1/appengine/cmd/milo/buildbot...
appengine/cmd/milo/buildbot/build.go:370: URL:   fmt.Sprintf("%s/%d", rietveld,
issue),
On 2016/06/28 02:06:44, nodir wrote:
> What if rietveld ends with /
> Trim it 

Done.

https://codereview.chromium.org/2102903002/diff/20001/appengine/cmd/milo/buil...
File appengine/cmd/milo/buildbot/build.go (right):

https://codereview.chromium.org/2102903002/diff/20001/appengine/cmd/milo/buil...
appengine/cmd/milo/buildbot/build.go:352: key := prop[0].(string)
On 2016/06/28 21:44:24, nodir wrote:
> please add a comment to buildbotBuild.Properties that each propetry is a slice
> of [propertyName, value, source] where source is optional and where
propertyName
> and source (if present) are always a string.
> 
> otherwise it looks like this will panic too
> 
> in general I wish there was a struct buildbotProperty that implements
> json.Unmarshaler https://godoc.org/encoding/json#Unmarshaler
> this way our code would remain type-safe

Done.

[Ryan Tseng, 2016-06-28 21:49:36]

The CQ bit was checked by hinoka@google.com

[commit-bot: I haz the power, 2016-06-28 21:49:48]

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-06-28 22:31:00]

Description was changed from

==========
Milo: Extract the rietveld information from buildbot.

So that the CL link can bubble up.

BUG=623372
==========

to

==========
Milo: Extract the rietveld information from buildbot.

So that the CL link can bubble up.

BUG=623372

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

[commit-bot: I haz the power, 2016-06-28 22:31:01]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/a547622d6472fbe5976d8c030936355464b1316f