Milo: Remove swarming tags from build properties

milo/appengine/swarming/build.go

Index: milo/appengine/swarming/build.go
diff --git a/milo/appengine/swarming/build.go b/milo/appengine/swarming/build.go
index be8b356f72ba9ab267cf81f2e6bbac006b725c3d..402f41c90d548b9e6437105c33703c8601381ccc 100644
--- a/milo/appengine/swarming/build.go
+++ b/milo/appengine/swarming/build.go
@@ -170,50 +170,32 @@ func taskProperties(sr *swarming.SwarmingRpcsTaskResult) *resp.PropertyGroup {
 	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{
@@ -224,20 +206,13 @@ func addBuilderLink(c context.Context, build *resp.MiloBuild, swarmingHostname s
 }
 
 // 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
@@ -348,24 +323,66 @@ func addTaskToMiloStep(c context.Context, server string, sr *swarming.SwarmingRp
 	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 != "" {