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.

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

s/Prepare/Modify/

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

Done.

+        updatePostProcessBuild(b)
+
         // 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
 }
+
+// updatePostProcessBuild transforms a build from its raw JSON format into the
+// format that should be presented to users.
+//
+// Post-processing includes:
+//      - If the build is LogDog-only, promotes aliases (LogDog links) to
+//        first-class links in the build.
+func updatePostProcessBuild(b *buildbotBuild) {
+        // If this is a LogDog-only build, we want to promote the LogDog links.
+        if loc, ok := b.getPropertyValue("log_location").(string); ok && strings.HasPrefix(loc, "logdog://") {
+                for sidx := range b.Steps {
+                        promoteLogDogOnlyLinksForBuildStep(&b.Steps[sidx], sidx == 0)
+                }
+        }
+}
+
+// promoteLogDogOnlyLinksForBuildStep updates the links in a BuildBot build to

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

in a BuildBot step, not build

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

Done.

+// 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 promoteLogDogOnlyLinksForBuildStep(s *buildbotStep, isInitialStep bool) {

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

nit: this is really long and.  how about just promoteLogDogLinks(...)

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

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

Done.

+        type stepLog struct {
+                label string
+                link  string

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

s/link/URL/

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

Done.

+        }
+
+        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 && isInitialStep && sl.label == "stdio" {
+                        // 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 {

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

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]

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.

+                newAliases = make(map[string][]*buildbotLinkAlias, l)
+                remainingAliases.Iter(func(v string) bool {
+                        newAliases[v] = s.Aliases[v]
+                        return true
+                })
+        }
+        s.Aliases = newAliases
+}