Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2991243002:
Milo: Console improvements (Closed)
|
Created:
3 years, 4 months ago by Ryan Tseng Modified:
3 years, 4 months ago CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
DescriptionMilo: Console improvements
* Add build link to console build item
* Bring back categories and shortnames
* Allow user to specify ?limit=
BUG=468053
Review-Url: https://codereview.chromium.org/2991243002
Committed: https://github.com/luci/luci-go/commit/481a3f1aca567a50b5c42f87569619f72069f721
Patch Set 1 #Patch Set 2 : Small fixes #
Total comments: 23
Patch Set 3 : Review #
Total comments: 6
Patch Set 4 : review #Patch Set 5 : Review comments #
Messages
Total messages: 34 (25 generated)
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Milo: Console improvements BUG= ========== to ========== Milo: Console improvements BUG=468053 ==========
Description was changed from ========== Milo: Console improvements BUG=468053 ========== to ========== Milo: Console improvements * Add build link to console build item * Bring back categories and shortnames * Allow user to specify ?limit= BUG=468053 ==========
hinoka@google.com changed reviewers: + iannucci@chromium.org
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal, check it out: https://luci-milo-dev.appspot.com/console/infra-experimental/fullmain?limit=200 (shortname/category alignment could use work, but I think it's representative now and could be iterated on)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hinoka@google.com changed reviewers: + dnj@chromium.org
https://codereview.chromium.org/2991243002/diff/20001/milo/common/config.go File milo/common/config.go (right): https://codereview.chromium.org/2991243002/diff/20001/milo/common/config.go#n... milo/common/config.go:105: func BuilderRefFromProto(cb []*config.Builder) []BuilderMeta { nit: function name needs to match comment https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/appengine... File milo/frontend/appengine/templates/pages/console.html (right): https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/appengine... milo/frontend/appengine/templates/pages/console.html:36: <a href="{{ .SelfLink }}" {{ . }}>x</a> {{ . }} https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... File milo/frontend/view_console.go (right): https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:55: func shortname(short, long string) string { What is "short" and "long" expected to be? Can you detail this in the comment? https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:55: func shortname(short, long string) string { This "short" logic is weird. IMO, just have the caller do: name := meta.ShortName if name == "" { name = shortname(b) } https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:59: builderName := strings.SplitN(long, "/", 3)[2] 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... https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:72: short += string(tokens[i][0]) Why the string cast? https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:72: short += string(tokens[i][0]) 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. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:95: builderRefs := make([]resp.BuilderRef, len(def.Builders)) Define this down near the "for" loop that uses it? https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:183: result += "<tr><th></th><th></th>" 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. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:187: builderName = err.Error() This seems not very user-friendly :x Perhaps set the builder name to "ERR" and have hovertext with the error body? https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:213: limit := 25 Define this as a constant at the top? https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:215: limit = tLimit Should we apply an upper bound here?
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal https://codereview.chromium.org/2991243002/diff/20001/milo/common/config.go File milo/common/config.go (right): https://codereview.chromium.org/2991243002/diff/20001/milo/common/config.go#n... milo/common/config.go:105: func BuilderRefFromProto(cb []*config.Builder) []BuilderMeta { On 2017/08/02 23:33:04, dnj wrote: > nit: function name needs to match comment oops copypasta mistake. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/appengine... File milo/frontend/appengine/templates/pages/console.html (right): https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/appengine... milo/frontend/appengine/templates/pages/console.html:36: <a href="{{ .SelfLink }}" {{ . }}>x</a> On 2017/08/02 23:33:04, dnj wrote: > {{ . }} This wasn't supposed to be here. Done. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... File milo/frontend/view_console.go (right): https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:55: func shortname(short, long string) string { On 2017/08/02 23:33:04, dnj wrote: > What is "short" and "long" expected to be? Can you detail this in the comment? Done. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:59: builderName := strings.SplitN(long, "/", 3)[2] On 2017/08/02 23:33:05, dnj wrote: > 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... Done. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:72: short += string(tokens[i][0]) On 2017/08/02 23:33:05, dnj wrote: > Why the string cast? Done. And apparently the index of a string is a rune or byte.... https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:95: builderRefs := make([]resp.BuilderRef, len(def.Builders)) On 2017/08/02 23:33:05, dnj wrote: > Define this down near the "for" loop that uses it? Done. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:183: result += "<tr><th></th><th></th>" On 2017/08/02 23:33:04, dnj wrote: > 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. Done. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:187: builderName = err.Error() On 2017/08/02 23:33:05, dnj wrote: > This seems not very user-friendly :x Perhaps set the builder name to "ERR" and > have hovertext with the error body? Good idea. Done. https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:213: limit := 25 On 2017/08/02 23:33:04, dnj wrote: > Define this as a constant at the top? If it's not reused it's better to be defined closer to the code (M-A style coding) https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:215: limit = tLimit On 2017/08/02 23:33:05, dnj wrote: > Should we apply an upper bound here? Okay 1000 it is
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... File milo/frontend/view_console.go (right): https://codereview.chromium.org/2991243002/diff/20001/milo/frontend/view_cons... milo/frontend/view_console.go:213: limit := 25 On 2017/08/03 00:46:12, Ryan Tseng wrote: > On 2017/08/02 23:33:04, dnj wrote: > > Define this as a constant at the top? > > If it's not reused it's better to be defined closer to the code (M-A style > coding) Meh fine, then you can inline: const maxLimit = 1000 const defaultLimit = 25 ... https://codereview.chromium.org/2991243002/diff/40001/milo/frontend/view_cons... File milo/frontend/view_console.go (right): https://codereview.chromium.org/2991243002/diff/40001/milo/frontend/view_cons... milo/frontend/view_console.go:75: if len(tokens[i]) > 0 { Note that if you have five tokens, "1--2-3-4", this will result in "12", not "123", since the skipped empty token will still count as one of the "numLetters". WDYT about this technique: pos := 0 for i := 0; i < len(tokens) && pos < 3; i++ { if tok := tokens[i]; len(tok) > 0 { tokens[pos] = tok pos++ } } return strings.Join(tokens[:pos], "") https://codereview.chromium.org/2991243002/diff/40001/milo/frontend/view_cons... milo/frontend/view_console.go:201: must(fmt.Fprintf(&buf, "<th><a title=\"%s\">ERR</a></th>", err.Error())) Nit: should probably escape the error string in case it has some weird characters in it: https://golang.org/pkg/html/template/#HTMLEscapeString https://codereview.chromium.org/2991243002/diff/40001/milo/frontend/view_cons... milo/frontend/view_console.go:205: br.Name, builderName, br.ShortName)) (*probably* safe not to escape these, although a malicious builder name could totally XSS you, builder names are defined by trusted actors.
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2991243002/diff/40001/milo/frontend/view_cons... File milo/frontend/view_console.go (right): https://codereview.chromium.org/2991243002/diff/40001/milo/frontend/view_cons... milo/frontend/view_console.go:75: if len(tokens[i]) > 0 { On 2017/08/03 01:02:59, dnj wrote: > Note that if you have five tokens, "1--2-3-4", this will result in "12", not > "123", since the skipped empty token will still count as one of the > "numLetters". > > WDYT about this technique: > pos := 0 > for i := 0; i < len(tokens) && pos < 3; i++ { > if tok := tokens[i]; len(tok) > 0 { > tokens[pos] = tok > pos++ > } > } > return strings.Join(tokens[:pos], "") Actually, playing around with it for a bit, it seems like fieldsfunc is smart enough to discard empty tokens: https://play.golang.org/p/MEtpn7O7hO So the empty string check is actually unnecessary https://codereview.chromium.org/2991243002/diff/40001/milo/frontend/view_cons... milo/frontend/view_console.go:201: must(fmt.Fprintf(&buf, "<th><a title=\"%s\">ERR</a></th>", err.Error())) On 2017/08/03 01:02:59, dnj wrote: > Nit: should probably escape the error string in case it has some weird > characters in it: https://golang.org/pkg/html/template/#HTMLEscapeString Done. https://codereview.chromium.org/2991243002/diff/40001/milo/frontend/view_cons... milo/frontend/view_console.go:205: br.Name, builderName, br.ShortName)) On 2017/08/03 01:02:59, dnj wrote: > (*probably* safe not to escape these, although a malicious builder name could > totally XSS you, builder names are defined by trusted actors. Better safe than sorry.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hinoka@google.com
The CQ bit was checked by hinoka@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org Link to the patchset: https://codereview.chromium.org/2991243002/#ps80001 (title: "Review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1501774877290800,
"parent_rev": "258f0c01c308e369ef4d9c96cfddfb25e7944ee9", "commit_rev":
"481a3f1aca567a50b5c42f87569619f72069f721"}
Message was sent while issue was closed.
Description was changed from ========== Milo: Console improvements * Add build link to console build item * Bring back categories and shortnames * Allow user to specify ?limit= BUG=468053 ========== to ========== Milo: Console improvements * Add build link to console build item * Bring back categories and shortnames * Allow user to specify ?limit= BUG=468053 Review-Url: https://codereview.chromium.org/2991243002 Committed: https://github.com/luci/luci-go/commit/481a3f1aca567a50b5c42f87569619f72069f721 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/luci/luci-go/commit/481a3f1aca567a50b5c42f87569619f72069f721 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
