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 6398f5921bad890384981000c879565bd0918ac5..db7d6a305b23d87005b9f6989b92ca45f5dbe499 100644
--- a/milo/appengine/swarming/build.go
+++ b/milo/appengine/swarming/build.go
@@ -247,41 +247,25 @@ 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{}

[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 {
-			p.Value = parts[1]
+			result[parts[0]] = 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,
@@ -291,20 +275,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, 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
@@ -415,24 +392,64 @@ func addTaskToMiloStep(c context.Context, server string, sr *swarming.SwarmingRp
 	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 != "" {