Promote LogDog aliases for BuildBot JSON data.

Promote LogDog aliases for BuildBot JSON data.

Generalize the LogDog alias-to-link promotion logic and apply it to the
BuildBot JSON endpoints. This will cause LogDog-only builds to omit
junk BuildBot log links in favor of the LogDog link aliases, bringing
the JSON in line with the rendered build.

Because Gatekeeper uses Milo for BuildBot step information, this will
incidentally cause Gatekeeper to include LogDog links in its failure
e-mails instead of the junk BuildBot links.

BUG=chromium:701201
TEST=expectations
  - Expectations, which already expressed this logic, didn't change.

R=hinoka@chromium.org

Review-Url: https://codereview.chromium.org/2774343003
Committed: https://github.com/luci/luci-go/commit/bd7c09a3a39523473f59ceb432b4f1dbc283c5c7

[hinoka, 2017-03-27 20:14:36]

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

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:497: postProcessBuild(b)
I like
b = postProcessBuild(b)

to make it crystal clear that there is a side effect (it can return the same
build).

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:510: // that should be presented to users.
Add to comment:
// This does the following:
// 1. Rewrites log links if logdog only mode is on.

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:515: for sidx := range b.Steps {
nit: just "i" is fine.

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:516: promoteLogDogOnlyLinksForStep(b, sidx,
&b.Steps[sidx])
I think:
b.Steps[i] = logDogOnlyStep(...)
is a cleaner interface

and then make the function return a Step

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:535: func promoteLogDogOnlyLinksForStep(b
*buildbotBuild, sidx int, s *buildbotStep) {
how about logDogOnlyStep(...)

Make sure comment for function matches function name.

Also looks like b *buildbotBuild is unused, remove that, so this should look
like:

func logDogOnlyStep(s *buildbotStep) (*buildbotStep) {...}

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:550: if isLog && sidx == 0 && sl.label ==
"stdio" {
This looks like the only place sidx is used.  Consider pulling this logic out to
L516 so that the function at L535 only deals with a single step

[dnj, 2017-03-27 21:31:33]

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

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:497: postProcessBuild(b)
On 2017/03/27 20:14:35, hinoka wrote:
> I like
> b = postProcessBuild(b)
> 
> to make it crystal clear that there is a side effect (it can return the same
> build).

Talked offline, making "updatePostProcessBuild".

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:510: // that should be presented to users.
On 2017/03/27 20:14:35, hinoka wrote:
> Add to comment:
> // This does the following:
> // 1. Rewrites log links if logdog only mode is on.

Done.

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:515: for sidx := range b.Steps {
On 2017/03/27 20:14:35, hinoka wrote:
> nit: just "i" is fine.

I want to use the same variable name as in "promoteLogDogOnlyLinksForBuildStep"
just for consistency.

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:516: promoteLogDogOnlyLinksForStep(b, sidx,
&b.Steps[sidx])
On 2017/03/27 20:14:35, hinoka wrote:
> I think:
> b.Steps[i] = logDogOnlyStep(...)
> is a cleaner interface
> 
> and then make the function return a Step

That notation implies that a correct implementation involves cloning the Step
and returning a new one. Returning the same thing is redundant.

Additionally, since this is a buildbotStep, not a *buildbotStep, this actually
involves a shallow clone.

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:535: func promoteLogDogOnlyLinksForStep(b
*buildbotBuild, sidx int, s *buildbotStep) {
On 2017/03/27 20:14:35, hinoka wrote:
> how about logDogOnlyStep(...)
> 
> Make sure comment for function matches function name.
> 
> Also looks like b *buildbotBuild is unused, remove that, so this should look
> like:
> 
> func logDogOnlyStep(s *buildbotStep) (*buildbotStep) {...}

Chatted offline, made "promoteLogDogOnlyLinksForBuildStep".

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:550: if isLog && sidx == 0 && sl.label ==
"stdio" {
On 2017/03/27 20:14:35, hinoka wrote:
> This looks like the only place sidx is used.  Consider pulling this logic out
to
> L516 so that the function at L535 only deals with a single step

Replaced with "isInitialStep" boolean.

[hinoka, 2017-03-27 22:04:26]

lgtm + comments

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

https://codereview.chromium.org/2774343003/diff/1/milo/appengine/buildbot/bui...
milo/appengine/buildbot/build.go:515: for sidx := range b.Steps {
On 2017/03/27 21:31:33, dnj wrote:
> On 2017/03/27 20:14:35, hinoka wrote:
> > nit: just "i" is fine.
> 
> I want to use the same variable name as in
"promoteLogDogOnlyLinksForBuildStep"
> just for consistency.

Acknowledged.

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

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:496: // Prepare the build for rendering.
s/Prepare/Modify/

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:524: // promoteLogDogOnlyLinksForBuildStep
updates the links in a BuildBot build to
in a BuildBot step, not build

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:538: func promoteLogDogOnlyLinksForBuildStep(s
*buildbotStep, isInitialStep bool) {
nit: this is really long and.  how about just promoteLogDogLinks(...)

buildbotStep is potentially implicit since it's part of the function signature.

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:541: link  string
s/link/URL/

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:614: if l := remainingAliases.Len(); l > 0 {
How about just

newAliases := make(map[string][]*buildbotLinkAlias, remainingAliases)
for v, ok := remainingAliases.Pop() ; ok {
  newAliases[v] = s.Aliases[V]
}

[dnj, 2017-03-27 22:20:16]

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

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:496: // Prepare the build for rendering.
On 2017/03/27 22:04:26, hinoka wrote:
> s/Prepare/Modify/

Done.

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:524: // promoteLogDogOnlyLinksForBuildStep
updates the links in a BuildBot build to
On 2017/03/27 22:04:26, hinoka wrote:
> in a BuildBot step, not build

Done.

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:538: func promoteLogDogOnlyLinksForBuildStep(s
*buildbotStep, isInitialStep bool) {
On 2017/03/27 22:04:26, hinoka wrote:
> nit: this is really long and.  how about just promoteLogDogLinks(...)
> 
> buildbotStep is potentially implicit since it's part of the function
signature.

Done.

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:541: link  string
On 2017/03/27 22:04:26, hinoka wrote:
> s/link/URL/

Done.

https://codereview.chromium.org/2774343003/diff/20001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:614: if l := remainingAliases.Len(); l > 0 {
On 2017/03/27 22:04:26, hinoka wrote:
> How about just
> 
> newAliases := make(map[string][]*buildbotLinkAlias, remainingAliases)
> for v, ok := remainingAliases.Pop() ; ok {
>   newAliases[v] = s.Aliases[V]
> }

I like to not allocate a map at all (not even one with zero/default capacity) if
there are no elements that are going to be in it. This is doubly important here,
since having leftover aliases is not expected to be the common case.

[dnj, 2017-03-27 22:20:31]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-03-27 22:20:32]

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/2774343003/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2017-03-27 22:20:37]

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, 2017-03-27 22:31:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490653231847080,
"parent_rev": "cb672ae7ae5be63e439de79cb0761299e888d00a", "commit_rev":
"bd7c09a3a39523473f59ceb432b4f1dbc283c5c7"}

[commit-bot: I haz the power, 2017-03-27 22:31:59]

Description was changed from

==========
Promote LogDog aliases for BuildBot JSON data.

Generalize the LogDog alias-to-link promotion logic and apply it to the
BuildBot JSON endpoints. This will cause LogDog-only builds to omit
junk BuildBot log links in favor of the LogDog link aliases, bringing
the JSON in line with the rendered build.

Because Gatekeeper uses Milo for BuildBot step information, this will
incidentally cause Gatekeeper to include LogDog links in its failure
e-mails instead of the junk BuildBot links.

BUG=chromium:701201
TEST=expectations
  - Expectations, which already expressed this logic, didn't change.

R=hinoka@chromium.org
==========

to

==========
Promote LogDog aliases for BuildBot JSON data.

Generalize the LogDog alias-to-link promotion logic and apply it to the
BuildBot JSON endpoints. This will cause LogDog-only builds to omit
junk BuildBot log links in favor of the LogDog link aliases, bringing
the JSON in line with the rendered build.

Because Gatekeeper uses Milo for BuildBot step information, this will
incidentally cause Gatekeeper to include LogDog links in its failure
e-mails instead of the junk BuildBot links.

BUG=chromium:701201
TEST=expectations
  - Expectations, which already expressed this logic, didn't change.

R=hinoka@chromium.org

Review-Url: https://codereview.chromium.org/2774343003
Committed:
https://github.com/luci/luci-go/commit/bd7c09a3a39523473f59ceb432b4f1dbc283c5c7
==========

[commit-bot: I haz the power, 2017-03-27 22:32:01]

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