Milo: Console improvements

milo/frontend/view_console.go

 // Copyright 2017 The LUCI Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //      http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
 package frontend
 
 import (
         "encoding/hex"
         "fmt"
         "html/template"
         "net/http"
+        "strings"
 
         "golang.org/x/net/context"
 
         "github.com/luci/luci-go/common/clock"
         "github.com/luci/luci-go/common/errors"
         "github.com/luci/luci-go/common/logging"
         "github.com/luci/luci-go/common/proto/google"
         "github.com/luci/luci-go/server/router"
         "github.com/luci/luci-go/server/templates"
 
@@ skipping 10 matching lines @@
 // that builder will be removed from list of results.
 func getConsoleDef(c context.Context, project, name string) (*common.Console, error) {
         cs, err := common.GetConsole(c, project, name)
         if err != nil {
                 return nil, err
         }
         // TODO(hinoka): Remove builders that the user does not have access to.
         return cs, nil
 }
 
-func console(c context.Context, project, name string) (*resp.Console, error) {
+// shortname returns the short if given, otherwise tries to calculate
+// a short name out of a long.
+func shortname(short, long string) string {

[dnj, 2017/08/02 23:33:04]

This "short" logic is weird. IMO, just have the caller do:

name := meta.ShortName
if name == "" {
  name = shortname(b)
}

[dnj, 2017/08/02 23:33:04]

What is "short" and "long" expected to be? Can you detail this in the comment?

[Ryan Tseng, 2017/08/03 00:46:12]

Done.

+        if short != "" {
+                return short
+        }
+        builderName := strings.SplitN(long, "/", 3)[2]

[dnj, 2017/08/02 23:33:05]

This makes the assumption that "long" has at least two "/" in it. Let's test
length first here. Alternatively, you could do "strings.Split(...)" (not SplitN)
and get the last element. Or flatten the string if it doesn't have two
slashes...

[Ryan Tseng, 2017/08/03 00:46:12]

Done.

+        tokens := strings.FieldsFunc(builderName, func(r rune) bool {
+                switch r {
+                case '_', '-', ' ':
+                        return true
+                }
+                return false
+        })
+        numLetters := len(tokens)
+        if numLetters > 3 {
+                numLetters = 3
+        }
+        for i := 0; i < numLetters; i++ {
+                short += string(tokens[i][0])

[dnj, 2017/08/02 23:33:05]

If the string has two delimiters (e.g., "--"), wouldn't one of the tokens be
empty? In this case, [0] will panic. Add logic to skip tokens[i] if
len(tokens[i]) == 0.

[dnj, 2017/08/02 23:33:05]

Why the string cast?

[Ryan Tseng, 2017/08/03 00:46:12]

Done.  And apparently the index of a string is a rune or byte....

+        }
+        return strings.ToLower(short)
+}
+
+func console(c context.Context, project, name string, limit int) (*resp.Console, error) {
         tStart := clock.Now(c)
         def, err := getConsoleDef(c, project, name)
         if err != nil {
                 return nil, err
         }
-        commitInfo, err := git.GetHistory(c, def.RepoURL, def.Ref, 25)
+        commitInfo, err := git.GetHistory(c, def.RepoURL, def.Ref, limit)
         if err != nil {
                 return nil, err
         }
         tGitiles := clock.Now(c)
         logging.Debugf(c, "Loading commits took %s.", tGitiles.Sub(tStart))
 
-        builderNames := make([]string, len(def.Builders))
-        builders := make([]resp.BuilderRef, len(def.Builders))
-        for i, b := range def.Builders {
-                builderNames[i] = b
-                builders[i].Name = b
-                _, _, builders[i].ShortName, _ = buildsource.BuilderID(b).Split()
-                // TODO(hinoka): Add Categories back in.
-        }
-
         commitNames := make([]string, len(commitInfo.Commits))
         for i, commit := range commitInfo.Commits {
                 commitNames[i] = hex.EncodeToString(commit.Hash)
         }
-        rows, err := buildsource.GetConsoleRows(c, project, def, commitNames, builderNames)
+
+        builderRefs := make([]resp.BuilderRef, len(def.Builders))

[dnj, 2017/08/02 23:33:05]

Define this down near the "for" loop that uses it?

[Ryan Tseng, 2017/08/03 00:46:12]

Done.

+        rows, err := buildsource.GetConsoleRows(c, project, def, commitNames, def.Builders)
         tConsole := clock.Now(c)
         logging.Debugf(c, "Loading the console took a total of %s.", tConsole.Sub(tGitiles))
         if err != nil {
                 return nil, err
         }
 
         ccb := make([]resp.CommitBuild, len(commitInfo.Commits))
         for row, commit := range commitInfo.Commits {
-                ccb[row].Build = make([]*model.BuildSummary, len(builders))
+                ccb[row].Build = make([]*model.BuildSummary, len(def.Builders))
                 ccb[row].Commit = resp.Commit{
                         AuthorName:  commit.AuthorName,
                         AuthorEmail: commit.AuthorEmail,
                         CommitTime:  google.TimeFromProto(commit.CommitTime),
                         Repo:        def.RepoURL,
                         Branch:      def.Ref, // TODO(hinoka): Actually this doesn't match, change branch to ref.
                         Description: commit.Msg,
                         Revision:    resp.NewLink(commitNames[row], def.RepoURL+"/+/"+commitNames[row]),
                 }
 
-                for col, b := range builders {
-                        name := buildsource.BuilderID(b.Name)
+                for col, b := range def.Builders {
+                        meta := def.BuilderMetas[col]
+                        builderRefs[col] = resp.BuilderRef{
+                                Name:      b,
+                                Category:  strings.Split(meta.Category, "|"),
+                                ShortName: shortname(meta.ShortName, b),
+                        }
+                        name := buildsource.BuilderID(b)
                         if summaries := rows[row].Builds[name]; len(summaries) > 0 {
                                 ccb[row].Build[col] = summaries[0]
                         }
                 }
         }
 
         return &resp.Console{
                 Name:       def.ID,
                 Commit:     ccb,
-                BuilderRef: builders,
+                BuilderRef: builderRefs,
         }, nil
 }
 
 // consoleRenderer is a wrapper around Console to provide additional methods.
 type consoleRenderer struct {
         *resp.Console
 }
 
 // Header generates the console header html.
 func (c consoleRenderer) Header() template.HTML {
@@ skipping 29 matching lines @@
                                 current = s
                         }
                 }
                 if colspan != 0 {
                         result += fmt.Sprintf(`<th colspan="%d">%s</th>`, colspan, current)
                 }
                 result += "</tr>"
         }
 
         // Last row: The actual builder shortnames.
         result += "<tr><th></th><th></th>"

[dnj, 2017/08/02 23:33:04]

nit: for this sort of construction, it's better to:

must := func(v int, err error) {
  if err != nil {
    panic(err)
  }
}

var buf bytes.Buffer
must(buf.WriteString(...))
must(fmt.Fprintf(&buf, ...)
return template.HTML(buf.String())


waaaaay fewer buffer allocation/copy.

[Ryan Tseng, 2017/08/03 00:46:13]

Done.

         for _, br := range c.BuilderRef {
-                result += fmt.Sprintf("<th>%s</th>", br.ShortName)
+                _, _, builderName, err := buildsource.BuilderID(br.Name).Split()
+                if err != nil {
+                        builderName = err.Error()

[dnj, 2017/08/02 23:33:05]

This seems not very user-friendly :x Perhaps set the builder name to "ERR" and
have hovertext with the error body?

[Ryan Tseng, 2017/08/03 00:46:13]

Good idea. Done.

+                }
+                result += fmt.Sprintf(
+                        "<th><a href=\"/%s\" title=\"%s\">%s</a></th>",
+                        br.Name, builderName, br.ShortName)
         }
         result += "</tr>"
         return template.HTML(result)
 }
 
 func (c consoleRenderer) BuilderLink(bs *model.BuildSummary) (*resp.Link, error) {
         _, _, builderName, err := buildsource.BuilderID(bs.BuilderID).Split()
         if err != nil {
                 return nil, err
         }
         return resp.NewLink(builderName, "/"+bs.BuilderID), nil
 }
 
 // ConsoleHandler renders the console page.
 func ConsoleHandler(c *router.Context) {
         project := c.Params.ByName("project")
         if project == "" {
                 ErrorHandler(c, errors.New("Missing Project", common.CodeParameterError))
                 return
         }
         name := c.Params.ByName("name")
+        limit := 25

[dnj, 2017/08/02 23:33:04]

Define this as a constant at the top?

[Ryan Tseng, 2017/08/03 00:46:12]

If it's not reused it's better to be defined closer to the code (M-A style
coding)

[dnj, 2017/08/03 01:02:58]

Meh fine, then you can inline:

const maxLimit = 1000
const defaultLimit = 25

...

+        if tLimit := GetLimit(c.Request, -1); tLimit >= 0 {
+                limit = tLimit

[dnj, 2017/08/02 23:33:05]

Should we apply an upper bound here?

[Ryan Tseng, 2017/08/03 00:46:12]

Okay 1000 it is

+        }
 
-        result, err := console(c.Context, project, name)
+        result, err := console(c.Context, project, name, limit)
         if err != nil {
                 ErrorHandler(c, err)
                 return
         }
 
         templates.MustRender(c.Context, c.Writer, "pages/console.html", templates.Args{
                 "Console": consoleRenderer{result},
         })
 }
 
 // ConsoleMainHandler is a redirect handler that redirects the user to the main
 // console for a particular project.
 func ConsoleMainHandler(ctx *router.Context) {
         w, r, p := ctx.Writer, ctx.Request, ctx.Params
         proj := p.ByName("project")
         http.Redirect(w, r, fmt.Sprintf("/console/%s/main", proj), http.StatusMovedPermanently)
         return
 }