Milo: Print raw json for buildbot build properties

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"
         "io/ioutil"
+        "math"
         "path/filepath"
         "regexp"
         "sort"
         "strings"
         "time"
 
         "golang.org/x/net/context"
 
         "github.com/luci/gae/service/datastore"
         "github.com/luci/luci-go/common/data/stringset"
@@ skipping 284 matching lines @@
                 }
 
                 // Figure out the times.
                 bc.Started, bc.Finished, bc.Duration = parseTimes(endingTime, step.Times)
 
                 result = append(result, bc)
         }
         return
 }
 
 // parseProp returns a representation of v based off k, and a boolean to

[nodir, 2017/04/11 07:13:19]

there is no k

[nodir, 2017/04/11 07:13:19]

high level: i think JSON encoding should be in a template func, not here
because this logic is a part of presentation

[hinoka, 2017/04/13 21:01:59]

This was done this was because previously rietveld links required knowing both
the "rietveld" and "issue" fields (to match buildbot behavior).  This has been
since moved to the "Source" section.

 // specify whether or not to hide it altogether.

[nodir, 2017/04/11 07:13:19]

after I read this, i thought the function returns true if a property should be
hidden. 
maybe "whether or not to display it"? or more explicit ", and false to specify
to hide it altogether"

[hinoka, 2017/04/13 21:01:59]

Removed block.

-func parseProp(prop map[string]Prop, k string, v interface{}) (string, bool) {
-        switch k {
-        case "requestedAt":
-                if vf, ok := v.(float64); ok {
-                        return time.Unix(int64(vf), 0).UTC().Format(time.RFC3339), true
+func parseProp(v interface{}) (string, bool) {
+        if vs, ok := v.(string); ok && vs == "" {
+                return "", false // Value is empty, don't show it.

[nodir, 2017/04/11 07:13:19]

why we are hiding empty strings? They are legit values

[hinoka, 2017/04/13 21:01:59]

They why is because it seems kind of useless to display them.  But if you think
they should be there i can put it back.

[nodir, 2017/04/13 21:08:20]

Yeah, I think they should be there. Imagine someone triggers a build a
specifies a="" property. They would think they are doing something wrong if they
don't see the property in the list

+        }
+        if v == nil {
+                return "", false
+        }
+        // if v is a whole number, force it into an int.  json.Marshal() would turn
+        // it into what looks like a float instead.  We want this to remain and
+        // int instead of a number.
+        if vf, ok := v.(float64); ok {
+                if math.Floor(vf) == vf {
+                        return fmt.Sprintf("%d", int64(vf)), true
                 }
-        case "buildbucket":
-                var b map[string]interface{}
-                json.Unmarshal([]byte(v.(string)), &b)
-                if b["build"] == nil {
-                        return "", false
-                }
-                build := b["build"].(map[string]interface{})
-                id := build["id"]
-                if id == nil {
-                        return "", false
-                }
-                return fmt.Sprintf("https://cr-buildbucket.appspot.com/b/%s", id.(string)), true
-        case "issue":
-                if rv, ok := prop["rietveld"]; ok {
-                        rietveld := rv.Value.(string)
-                        // Issue could be a float, int, or string.
-                        switch v := v.(type) {
-                        case float64:
-                                return fmt.Sprintf("%s/%d", rietveld, int(v)), true
-                        default: // Probably int or string
-                                return fmt.Sprintf("%s/%v", rietveld, v), true
-                        }
-                }
-                return fmt.Sprintf("%d", int(v.(float64))), true
-        case "rietveld":
-                return "", false
-        default:
-                if vs, ok := v.(string); ok && vs == "" {
-                        return "", false // Value is empty, don't show it.
-                }
-                if v == nil {
-                        return "", false
-                }
+        }
+        // return the json representation of the value.
+        b, err := json.Marshal(v)
+        if err == nil {
+                return string(b), true
         }
         return fmt.Sprintf("%v", v), true
 }
 
 // Prop is a struct used to store a value and group so that we can make a map
 // of key:Prop to pass into parseProp() for the purpose of cross referencing
 // one prop while working on another.
 type Prop struct {
         Value interface{}
         Group string
 }
 
 // properties extracts all properties from buildbot builds and groups them into
 // property groups.
 func properties(b *buildbotBuild) (result []*resp.PropertyGroup) {
         groups := map[string]*resp.PropertyGroup{}
         allProps := map[string]Prop{}
         for _, prop := range b.Properties {
                 allProps[prop.Name] = Prop{
                         Value: prop.Value,
                         Group: prop.Source,
                 }
         }
         for key, prop := range allProps {
                 value := prop.Value
                 groupName := prop.Group
                 if _, ok := groups[groupName]; !ok {
                         groups[groupName] = &resp.PropertyGroup{GroupName: groupName}
                 }
-                vs, ok := parseProp(allProps, key, value)
+                vs, ok := parseProp(value)
                 if !ok {
                         continue
                 }
                 groups[groupName].Property = append(groups[groupName].Property, &resp.Property{
                         Key:   key,
                         Value: vs,

[nodir, 2017/04/11 07:13:20]

i think this should be an interface{} and JSON-encoding should be done via a
func in a template

[hinoka, 2017/04/13 21:01:59]

I'll do that in a different CL.  That would undermine a design decision that was
made in Milo. (ie, that properties are mean to be json serializable values meant
for machine consumption rather than human use.)

                 })
         }
         // Insert the groups into a list in alphabetical order.
         // You have to make a separate sorting data structure because Go doesn't like
         // sorting things for you.
         groupNames := []string{}
         for n := range groups {
                 groupNames = append(groupNames, n)
         }
         sort.Strings(groupNames)
@@ skipping 256 matching lines @@
         var newAliases map[string][]*buildbotLinkAlias
         if l := remainingAliases.Len(); l > 0 {
                 newAliases = make(map[string][]*buildbotLinkAlias, l)
                 remainingAliases.Iter(func(v string) bool {
                         newAliases[v] = s.Aliases[v]
                         return true
                 })
         }
         s.Aliases = newAliases
 }