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"
         "encoding/json"
         "fmt"
@@ skipping 152 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{}
         for _, t := range tags {
                 if t == "" {
                         continue
                 }
                 parts := strings.SplitN(t, ":", 2)
-                p := &resp.Property{
-                        Key: parts[0],
-                }
                 if len(parts) == 2 {
-                        p.Value = parts[1]
-                }
-                props.Property = append(props.Property, p)
-        }
-        return props
-}
-
-// tagValue returns a value of the first tag matching the tag key. If not found
-// returns "".
-func tagValue(tags []string, key string) string {
-        prefix := key + ":"
-        for _, t := range tags {
-                if strings.HasPrefix(t, prefix) {
-                        return strings.TrimPrefix(t, prefix)
+                        result[parts[0]] = parts[1]

[nodir, 2017/02/07 21:56:29]

are you sure tag keys are unique? add a func comment that this function assumes
that

consider logging an error if this is not the case

[hinoka, 2017/02/24 00:56:22]

Tags keys are supposed to be unique.  If a second step emits a conflicting key,
it gets overridden.

I'd rather not log here because i'd have to pass in a context to an otherwise
stateless function :(

                 }
         }
-        return ""
+        return result
 }
 
 // addBuilderLink adds a link to the buildbucket builder view.
-func addBuilderLink(c context.Context, build *resp.MiloBuild, swarmingHostname string, sr *swarming.SwarmingRpcsTaskResult) {
-        bbHost := tagValue(sr.Tags, "buildbucket_hostname")
-        bucket := tagValue(sr.Tags, "buildbucket_bucket")
-        builder := tagValue(sr.Tags, "builder")
+func addBuilderLink(c context.Context, build *resp.MiloBuild, selfURL string, props map[string]string) {

[nodir, 2017/02/07 21:56:30]

it is not obvious what "self" means here. Task? Consider changing to taskURL

[nodir, 2017/02/07 21:56:30]

s/props/tags/

in swarming land, properties mean something different

[hinoka, 2017/02/24 00:56:22]

Actually I just removed this entirely.  No need to log this since it'll be
evident by the lack of a builder link.  Also skia doesn't have this, so we'll
just get a bunch of error spam instead.  And now it looks cleaner.

[hinoka, 2017/02/24 00:56:22]

Done.

+        bbHost := props["buildbucket_hostname"]
+        bucket := props["buildbucket_bucket"]
+        builder := props["builder"]
         if bucket == "" {
                 logging.Errorf(
-                        c, "Could not extract buidlbucket bucket from task %s",
-                        taskPageURL(swarmingHostname, sr.TaskId))
+                        c, "Could not extract buidlbucket bucket from task %s", selfURL)
         }
         if builder == "" {
                 logging.Errorf(
-                        c, "Could not extract builder name from task %s",
-                        taskPageURL(swarmingHostname, sr.TaskId))
+                        c, "Could not extract builder name from task %s", selfURL)
         }
         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, props map[string]string) {

[nodir, 2017/02/07 21:56:29]

s/props/tags/

[hinoka, 2017/02/24 00:56:22]

Done.

+        os := props["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, props map[string]string) {

[nodir, 2017/02/07 21:56:29]

s/props/tags/

[hinoka, 2017/02/24 00:56:22]

Done.

+        buildset := props["buildset"]
+        if strings.HasPrefix(buildset, "patch/") {
+                if build.SourceStamp == nil {

[nodir, 2017/02/07 21:56:29]

ah, this is why an empty SourceStamp section appeared.
consider moving this to L337

[hinoka, 2017/02/24 00:56:22]

Done.

+                        build.SourceStamp = &resp.SourceStamp{}
+                }
+                patchset := strings.TrimLeft(buildset, "patch/")
+                if strings.HasPrefix(patchset, "gerrit/") {
+                        gerritset := strings.TrimLeft(patchset, "gerrit/")

[nodir, 2017/02/07 21:56:30]

gerritPatchset?

[hinoka, 2017/02/24 00:56:22]

Done.

+                        parts := strings.Split(gerritset, "/")
+                        if len(parts) == 3 {
+                                build.SourceStamp.Changelist = &resp.Link{
+                                        Label: "Gerrit CL",
+                                        URL:   fmt.Sprintf("https://%s/r/%s/%s", parts[0], parts[1], parts[2]),

[nodir, 2017/02/07 21:56:30]

never saw /r/ before. 
did you mean c?

[hinoka, 2017/02/24 00:56:22]

Oops fixed.

+                                }
+                        }
+                }
+        }
+}
+
+func addRecipeLink(build *resp.MiloBuild, props map[string]string) {

[nodir, 2017/02/07 21:56:30]

tags

[hinoka, 2017/02/24 00:56:22]

Done.

+        name := props["recipe_name"]
+        repo := props["recipe_repository"]
+        revision := props["recipe_revision"]
+        if name != "" && repo != "" {
+                subpath := "/+/"

[nodir, 2017/02/07 21:56:30]

move /+/ to the format string

[hinoka, 2017/03/01 22:53:10]

Done.

+                if revision != "" {
+                        subpath += revision + "/"
+                } else {
+                        subpath += "master/"
+                }
+                if repo == "https://chromium.googlesource.com/chromium/tools/build" {
+                        subpath += "scripts/slave/recipes/"

[nodir, 2017/02/07 21:56:29]

this isn't right

we should fetch recipes.cfg
(https://chromium.googlesource.com/infra/infra/+/master/infra/config/recipes.cfg)
to find the recipe_path

[hinoka, 2017/03/01 22:53:10]

That won't work, removing sutpath alltogether

+                }

[nodir, 2017/02/07 21:56:30]

this will not work for any other repos?

for https://luci-milo.appspot.com/swarming/task/342fe1d557939c10
URL will be https://chromium.googlesource.com/infra/infra/+/master/luci_go.py
which is invalid URL

[hinoka, 2017/03/01 22:53:10]

Acknowledged.

+                build.Summary.Recipe = &resp.Link{
+                        Label: name,
+                        URL:   fmt.Sprintf("%s%s%s.py", repo, subpath, name),
+                }
+        }
+}
+
 func addTaskToBuild(c context.Context, server string, sr *swarming.SwarmingRpcsTaskResult, build *resp.MiloBuild) error {
+        selfURL := taskPageURL(server, sr.TaskId)
         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)
-        }
+        props := tagsToMap(sr.Tags)

[nodir, 2017/02/07 21:56:30]

tags

[hinoka, 2017/03/01 22:53:10]

Done.

 
-        addBuilderLink(c, build, server, sr)
-        addBanner(build, sr)
+        addBuildsetInfo(build, props)
+        addBanner(build, props)
+        addBuilderLink(c, build, selfURL, props)
+        addRecipeLink(build, props)
 
         // 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 129 matching lines @@
         case *miloProto.Link_Url:
                 return &resp.Link{
                         Label: l.Label,
                         URL:   t.Url,
                 }
 
         default:
                 return nil
         }
 }