Milo: Remove swarming tags from build properties

milo/appengine/swarming/build.go

 // Copyright 2015 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 swarming
 
 import (
         "bytes"
         "fmt"
         "net/http"
@@ skipping 229 matching lines @@
         }
         if sr.State == TaskCompleted || sr.State == TaskTimedOut {
                 props.Property = append(props.Property, &resp.Property{
                         Key:   "Exit Code",
                         Value: fmt.Sprintf("%d", sr.ExitCode),
                 })
         }
         return props
 }
 
-func tagsToProperties(tags []string) *resp.PropertyGroup {
-        props := &resp.PropertyGroup{GroupName: "Swarming Tags"}
+func tagsToMap(tags []string) map[string]string {
+        result := map[string]string{}

[nodir, 2017/03/01 23:25:04]

preallocate len(tags) entries

[hinoka, 2017/03/02 03:44:40]

Done.

         for _, t := range tags {
                 if t == "" {

[nodir, 2017/03/01 23:25:04]

this if statement is unnecessary: strings.SpinN("", ":", 2) == []

[hinoka, 2017/03/02 03:44:40]

Done.

                         continue
                 }
                 parts := strings.SplitN(t, ":", 2)
-                p := &resp.Property{
-                        Key: parts[0],
+                if len(parts) == 2 {
+                        result[parts[0]] = parts[1]
                 }
-                if len(parts) == 2 {
-                        p.Value = parts[1]
-                }
-                props.Property = append(props.Property, p)
         }
-        return props
+        return result
 }
 
 // addBuilderLink adds a link to the buildbucket builder view.
-func addBuilderLink(c context.Context, build *resp.MiloBuild, swarmingHostname string, sr *swarming.SwarmingRpcsTaskResult) {
-        tags := swarmingTags(sr.Tags)
-
+func addBuilderLink(c context.Context, build *resp.MiloBuild, tags map[string]string) {
         bbHost := tags["buildbucket_hostname"]
         bucket := tags["buildbucket_bucket"]
         builder := tags["builder"]
-        if bucket == "" {
-                logging.Errorf(
-                        c, "Could not extract buildbucket bucket from task %s",
-                        taskPageURL(swarmingHostname, sr.TaskId))
-        }
-        if builder == "" {
-                logging.Errorf(
-                        c, "Could not extract builder name from task %s",
-                        taskPageURL(swarmingHostname, sr.TaskId))
-        }
         if bucket != "" && builder != "" {
                 build.Summary.ParentLabel = &resp.Link{
                         Label: builder,
                         URL:   fmt.Sprintf("/buildbucket/%s/%s?server=%s", bucket, builder, bbHost),
                 }
         }
 }
 
 // addBanner adds an OS banner derived from "os" swarming tag, if present.
-func addBanner(build *resp.MiloBuild, sr *swarming.SwarmingRpcsTaskResult) {
-        var os, ver string
-        for _, t := range sr.Tags {
-                value := strings.TrimPrefix(t, "os:")
-                if value == t {
-                        // t does not have the prefix
-                        continue
-                }
-                parts := strings.SplitN(value, "-", 2)
-                if len(parts) == 2 {
-                        os = parts[0]
-                        ver = parts[1]
-                        break
-                }
+func addBanner(build *resp.MiloBuild, tags map[string]string) {
+        os := tags["os"]
+        var ver string
+        parts := strings.SplitN(os, "-", 2)
+        if len(parts) == 2 {
+                os = parts[0]
+                ver = parts[1]
         }
 
         var base resp.LogoBase
         switch os {
         case "Ubuntu":
                 base = resp.Ubuntu
         case "Windows":
                 base = resp.Windows
         case "Mac":
                 base = resp.OSX
@@ skipping 90 matching lines @@
                 ts, err := time.Parse(SwarmingTimeLayout, sr.CompletedTs)
                 if err != nil {
                         return fmt.Errorf("invalid task CompletedTs: %s", err)
                 }
                 step.Ended = google.NewTimestamp(ts)
         }
 
         return nil
 }
 
+func addBuildsetInfo(build *resp.MiloBuild, tags map[string]string) {
+        buildset := tags["buildset"]
+        if strings.HasPrefix(buildset, "patch/") {
+                patchset := strings.TrimLeft(buildset, "patch/")
+                // TODO(hinoka): Also support Rietveld patches.
+                if strings.HasPrefix(patchset, "gerrit/") {
+                        gerritPatchset := strings.TrimLeft(patchset, "gerrit/")
+                        parts := strings.Split(gerritPatchset, "/")
+                        if len(parts) == 3 {
+                                if build.SourceStamp == nil {
+                                        build.SourceStamp = &resp.SourceStamp{}
+                                }
+                                build.SourceStamp.Changelist = &resp.Link{
+                                        Label: "Gerrit CL",
+                                        URL:   fmt.Sprintf("https://%s/c/%s/%s", parts[0], parts[1], parts[2]),
+                                }
+                        }
+                }
+        }
+}

[nodir, 2017/03/01 23:25:04]

that's a lot of nesting. Consider inverting your if statements and put returns

[hinoka, 2017/03/02 03:44:40]

Done.

+
+func addRecipeLink(build *resp.MiloBuild, tags map[string]string) {
+        name := tags["recipe_name"]

[nodir, 2017/03/01 23:25:04]

name is not used

[hinoka, 2017/03/02 03:44:40]

It's used as the label

[nodir, 2017/03/02 04:01:25]

oh, sorry, i am blind

+        repo := tags["recipe_repository"]
+        revision := tags["recipe_revision"]
+        if name != "" && repo != "" {
+                subpath := ""
+                if revision != "" {
+                        subpath += revision + "/"
+                } else {
+                        subpath += "master/"

[nodir, 2017/03/01 23:25:04]

it is more correct to use HEAD, not master

[hinoka, 2017/03/02 03:44:40]

HEAD doesn't make any sense on git if you don't specify a current branch or
commit.
See http://stackoverflow.com/questions/2304087/what-is-head-in-git
Also https://git-scm.com/book/en/v2/Git-Internals-Git-References
"The HEAD file is a symbolic reference to the branch you’re currently on"

/+/HEAD works probably because theres an implicit mapping that the current
branch is master, but that's an implicit thing rather than an explicit thing. 
It doesn't always imply the master branch.

We happen to use HEAD as a synonym to ToT on master because it is correct in
SVN.  The terminology carried over but it has a very subtly different meaning.

[nodir, 2017/03/02 04:01:25]

ack

+                }
+                build.Summary.Recipe = &resp.Link{
+                        Label: name,
+                        URL:   fmt.Sprintf("%s/+/%s", repo, subpath),

[nodir, 2017/03/01 23:25:04]

this works only on Gitiles. Let's not do this on non-gitiles. Please check if
the repository looks like a Gitiles URL, e.g. ends with .googlesource.com

[hinoka, 2017/03/02 03:44:40]

Done.

+                }
+        }
+}
+
 func addTaskToBuild(c context.Context, server string, sr *swarming.SwarmingRpcsTaskResult, build *resp.MiloBuild) error {
+        selfURL := taskPageURL(server, sr.TaskId)

[nodir, 2017/03/01 23:25:04]

s/selfURL/sourceURL/? or taskURL? self is ambiguous, i interpreted it as the URL
of the milo build page

[hinoka, 2017/03/02 03:44:40]

Actually I'll just inline it.

         build.Summary.Label = sr.TaskId
         build.Summary.Type = resp.Recipe
         build.Summary.Source = &resp.Link{
                 Label: "Task " + sr.TaskId,
-                URL:   taskPageURL(server, sr.TaskId),
+                URL:   selfURL,
         }
 
         // Extract more swarming specific information into the properties.
         if props := taskProperties(sr); len(props.Property) > 0 {
                 build.PropertyGroup = append(build.PropertyGroup, props)
         }
-        if props := tagsToProperties(sr.Tags); len(props.Property) > 0 {
-                build.PropertyGroup = append(build.PropertyGroup, props)
-        }
+        tags := tagsToMap(sr.Tags)
 
-        addBuilderLink(c, build, server, sr)
-        addBanner(build, sr)
+        addBuildsetInfo(build, tags)
+        addBanner(build, tags)
+        addBuilderLink(c, build, tags)
+        addRecipeLink(build, tags)
 
         // Add a link to the bot.
         if sr.BotId != "" {
                 build.Summary.Bot = &resp.Link{
                         Label: sr.BotId,
                         URL:   botPageURL(server, sr.BotId),
                 }
         }
 
         return nil
@@ skipping 261 matching lines @@
         for _, tag := range v {
                 var value string
                 parts := strings.SplitN(tag, ":", 2)
                 if len(parts) == 2 {
                         value = parts[1]
                 }
                 res[parts[0]] = value
         }
         return res
 }