Promote LogDog aliases for BuildBot JSON data.

milo/appengine/buildbot/build.go

 // Copyright 2016 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package buildbot
 
 import (
         "encoding/json"
         "errors"
         "fmt"
@@ skipping 21 matching lines @@
                 Master:      master,
                 Buildername: builder,
                 Number:      buildNum,
         }
 
         err := datastore.Get(c, result)
         err = checkAccess(c, err, result.Internal)
         if err == errMasterNotFound {
                 err = errBuildNotFound
         }
+
         return result, err
 }
 
 // result2Status translates a buildbot result integer into a resp.Status.
 func result2Status(s *int) (status resp.Status) {
         if s == nil {
                 return resp.Running
         }
         switch *s {
         case 0:
@@ skipping 122 matching lines @@
         }
 
         return sum
 }
 
 var rLineBreak = regexp.MustCompile("<br */?>")
 
 // components takes a full buildbot build struct and extract step info from all
 // of the steps and returns it as a list of milo Build Components.
 func components(b *buildbotBuild) (result []*resp.BuildComponent) {
-        for i, step := range b.Steps {
+        for _, step := range b.Steps {
                 if step.Hidden == true {
                         continue
                 }
                 bc := &resp.BuildComponent{
                         Label: step.Name,
                 }
                 // Step text sometimes contains <br>, which we want to parse into new lines.
                 for _, t := range step.Text {
                         for _, line := range rLineBreak.Split(t, -1) {
                                 bc.Text = append(bc.Text, line)
                         }
                 }
 
                 // Figure out the status.
                 if !step.IsStarted {
                         bc.Status = resp.NotRun
                 } else if !step.IsFinished {
                         bc.Status = resp.Running
                 } else {
                         if len(step.Results) > 0 {
                                 status := int(step.Results[0].(float64))
                                 bc.Status = result2Status(&status)
                         } else {
                                 bc.Status = resp.Success
                         }
                 }
 
-                // Now, link to the logs.
-                //
-                // Any given log may have an alias link. There may also be alises that are
-                // not attached to logs (unusual, but allowed), so we need to track which
-                // aliases are unreferenced and add them as immediate links.
-                //
-                // Additionally, if the build is LogDog-only ("log_location" is "logdog://")
-                // then we want to render *only* the alias for steps with aliases.
-                isLogDogOnly := false
-                if loc, ok := b.getPropertyValue("log_location").(string); ok && strings.HasPrefix(loc, "logdog://") {
-                        isLogDogOnly = true
-                }
                 remainingAliases := stringset.New(len(step.Aliases))
                 for linkAnchor := range step.Aliases {
                         remainingAliases.Add(linkAnchor)
                 }
 
                 getLinksWithAliases := func(logLink *resp.Link, isLog bool) resp.LinkSet {
                         // Generate alias links.
                         var aliases resp.LinkSet
                         if remainingAliases.Del(logLink.Label) {
                                 stepAliases := step.Aliases[logLink.Label]
                                 aliases = make(resp.LinkSet, len(stepAliases))
                                 for i, alias := range stepAliases {
                                         aliases[i] = alias.toLink()
                                 }
                         }
 
-                        var links resp.LinkSet
-                        if isLogDogOnly && len(aliases) > 0 && !(isLog && i == 0 && logLink.Label == "stdio") {
-                                // LogDog only and there are viable aliases. Feature them instead of the
-                                // log link.
-                                //
-                                // We exclude the top-level "stdio" link here because there isn't a
-                                // LogDog equivalent of the raw step output.
-                                //
-                                // Any link named "logdog" (Annotee cosmetic implementation detail) will
-                                // inherit the name of the build link.
-                                for _, a := range aliases {
-                                        if a.Label == "logdog" {
-                                                a.Label = logLink.Label
-                                        }
-                                }
-                                return aliases
-                        }
-
                         // Step log link takes primary, with aliases as secondary.
-                        links = make(resp.LinkSet, 1, 1+len(aliases))
+                        links := make(resp.LinkSet, 1, 1+len(aliases))
                         links[0] = logLink
 
                         for _, a := range aliases {
                                 a.Alias = true
                         }
                         return append(links, aliases...)
                 }
 
                 for _, l := range step.Logs {
                         logLink := resp.Link{
@@ skipping 245 matching lines @@
         var err error
         if master == "debug" {
                 b, err = getDebugBuild(c, builder, buildNum)
         } else {
                 b, err = getBuild(c, master, builder, buildNum)
         }
         if err != nil {
                 return nil, err
         }
 
+        // Prepare the build for rendering.
+        postProcessBuild(b)

[hinoka, 2017/03/27 20:14:35]

I like
b = postProcessBuild(b)

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

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

Talked offline, making "updatePostProcessBuild".

+
         // TODO(hinoka): Do all fields concurrently.
         return &resp.MiloBuild{
                 SourceStamp:   sourcestamp(c, b),
                 Summary:       summary(c, b),
                 Components:    components(b),
                 PropertyGroup: properties(b),
                 Blame:         blame(b),
         }, nil
 }
+
+// postProcessBuild transforms a build from its raw JSON format into the format
+// that should be presented to users.

[hinoka, 2017/03/27 20:14:35]

Add to comment:
// This does the following:
// 1. Rewrites log links if logdog only mode is on.

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

Done.

+func postProcessBuild(b *buildbotBuild) {
+        // If this is a LogDog-only build, we want to promote the LogDog links, which
+        // come in the form of aliases, to first-class build log and links.
+        if loc, ok := b.getPropertyValue("log_location").(string); ok && strings.HasPrefix(loc, "logdog://") {
+                for sidx := range b.Steps {

[hinoka, 2017/03/27 20:14:35]

nit: just "i" is fine.

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

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

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

Acknowledged.

+                        promoteLogDogOnlyLinksForStep(b, sidx, &b.Steps[sidx])

[hinoka, 2017/03/27 20:14:35]

I think:
b.Steps[i] = logDogOnlyStep(...)
is a cleaner interface

and then make the function return a Step

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

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.

+                }
+        }
+}
+
+// postProcessBuildPromoteLogDogOnly updates the links in a BuildBot build to
+// promote LogDog links.
+//
+// A build's links come in one of three forms:
+//      - Log Links, which link directly to BuildBot build logs.
+//      - URL Links, which are named links to arbitrary URLs.
+//      - Aliases, which attach to the label in one of the other types of links and
+//        augment it with additional named links.
+//
+// LogDog uses aliases exclusively to attach LogDog logs to other links. When
+// the build is LogDog-only, though, the original links are actually junk. What
+// we want to do is remove the original junk links and replace them with their
+// alias counterparts, so that the "natural" BuildBot links are actually LogDog
+// links.
+func promoteLogDogOnlyLinksForStep(b *buildbotBuild, sidx int, s *buildbotStep) {

[hinoka, 2017/03/27 20:14:35]

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) {...}

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

Chatted offline, made "promoteLogDogOnlyLinksForBuildStep".

+        type stepLog struct {
+                label string
+                link  string
+        }
+
+        remainingAliases := stringset.New(len(s.Aliases))
+        for linkAnchor := range s.Aliases {
+                remainingAliases.Add(linkAnchor)
+        }
+
+        maybePromoteAliases := func(sl *stepLog, isLog bool) []*stepLog {
+                // As a special case, if this is the first step ("steps" in BuildBot), we
+                // will refrain from promoting aliases for "stdio", since "stdio" represents
+                // the raw BuildBot logs.
+                if isLog && sidx == 0 && sl.label == "stdio" {

[hinoka, 2017/03/27 20:14:35]

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]

Replaced with "isInitialStep" boolean.

+                        // No aliases, don't modify this log.
+                        return []*stepLog{sl}
+                }
+
+                // If there are no aliases, we should obviously not promote them. This will
+                // be the case for pre-LogDog steps such as build setup.
+                aliases := s.Aliases[sl.label]
+                if len(aliases) == 0 {
+                        return []*stepLog{sl}
+                }
+
+                // We have chosen to promote the aliases. Therefore, we will not include
+                // them as aliases in the modified step.
+                remainingAliases.Del(sl.label)
+
+                result := make([]*stepLog, len(aliases))
+                for i, alias := range aliases {
+                        aliasStepLog := stepLog{alias.Text, alias.URL}
+
+                        // Any link named "logdog" (Annotee cosmetic implementation detail) will
+                        // inherit the name of the original log.
+                        if !isLog {
+                                if aliasStepLog.label == "logdog" {
+                                        aliasStepLog.label = sl.label
+                                }
+                        }
+
+                        result[i] = &aliasStepLog
+                }
+                return result
+        }
+
+        // Update step logs.
+        newLogs := make([][]string, 0, len(s.Logs))
+        for _, l := range s.Logs {
+                for _, res := range maybePromoteAliases(&stepLog{l[0], l[1]}, true) {
+                        newLogs = append(newLogs, []string{res.label, res.link})
+                }
+        }
+        s.Logs = newLogs
+
+        // Update step URLs.
+        newURLs := make(map[string]string, len(s.Urls))
+        for label, link := range s.Urls {
+                urlLinks := maybePromoteAliases(&stepLog{label, link}, false)
+                if len(urlLinks) > 0 {
+                        // Use the last URL link, since our URL map can only tolerate one link.
+                        // The expected case here is that len(urlLinks) == 1, though, but it's
+                        // possible that multiple aliases can be included for a single URL, so
+                        // we need to handle that.
+                        newValue := urlLinks[len(urlLinks)-1]
+                        newURLs[newValue.label] = newValue.link
+                } else {
+                        newURLs[label] = link
+                }
+        }
+        s.Urls = newURLs
+
+        // Preserve any aliases that haven't been promoted.
+        var newAliases map[string][]*buildbotLinkAlias
+        if l := remainingAliases.Len(); l > 0 {
+                newAliases = make(map[string][]*buildbotLinkAlias, l)
+                remainingAliases.Iter(func(v string) bool {
+                        newAliases[v] = s.Aliases[v]
+                        return true
+                })
+        }
+        s.Aliases = newAliases
+}