[milo] Refactor all html knowledge out of backends.

[milo] Refactor all html knowledge out of backends.

R=hinoka@chromium.org, estaab@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2977863002
Committed: https://github.com/luci/luci-go/commit/3fa2a52a950e10b4eaac37b839966dd1874f5777

[iannucci, 2017-07-13 03:33:00]

Description was changed from

==========
[milo] Refactor all html knowledge out of backends.

R=hinoka@chromium.org
BUG=
==========

to

==========
[milo] Refactor all html knowledge out of backends.

R=hinoka@chromium.org, estaab@chromium.org
BUG=
==========

[iannucci, 2017-07-13 03:33:00]

iannucci@chromium.org changed reviewers:
+ estaab@chromium.org

[iannucci, 2017-07-13 03:33:19]

formal PTAL :)

This is running here:
https://1828-01276bd-tainted-iannucci-dot-luci-milo-dev.appspot.com/

[Ryan Tseng, 2017-07-13 22:00:45]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2017-07-13 22:00:47]

lgtm ! with some suggestion comments.

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
File milo/buildsource/buildbot/build.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbot/build.go:675: // BuildID is buildbots's notion of a
Build. See buildsource.ID.
Make a note of why there is an error in here. (Because we validate on
generation, but return the error only when Get() is called)

It does seem a little awkward though, I have a very slight preference towards
making Number an string just so we don't have to carry the error around.

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
File milo/buildsource/buildbot/builder.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbot/builder.go:228: ).Tag(common.CodeParameterError).Err()
404?

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
File milo/buildsource/buildbucket/builder.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbucket/builder.go:311: // TODO(iannucci): set buildbucket
host?
buildbucket:milo is a 1:1 relationship atm, so it's not needed currently.

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbucket/builder.go:312: var query builderQuery
nit: inline

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/builde...
File milo/buildsource/builder.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/builde...
milo/buildsource/builder.go:28: func parseTwo(path, nameType, nameA, nameB
string) (a, b string, err error) {
docstring - what is path? what is nameType? nameA? nameB? what does this return?
What is it for?

Actually this looks like a glorified SplitN(..., 2).  I think it's better to get
rid of this and do everything inline in Get(), or create another internal method
func (b BuilderID) Parse() (moduleName, builderGroupName, builderName string)
{...} would be a better idea.  Becaue:
1. The existence of parseTwo is confusing.  The function signature doesn't lead
it to imply that it is good at any particular job.
2. The error message are confusing.  This would say "bad BuilderID: cannot parse
source token path" when in reality what we want to say is "bad BuilderID: path
has less than 3 components", or "badBuilderID: Missing builder name"

I think this would be much simpler as a:
func (b builderID) parse() (moduleName, builderGroupName, builderName string)
{...}

your choice though

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/swarmi...
File milo/buildsource/swarming/build.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/swarmi...
milo/buildsource/swarming/build.go:823: func (b *BuildID) Get(ctx
context.Context) (*resp.MiloBuild, error) {
nit: conventionally Milo uses "ctx" for router.Context and "c" for
context.Context.

https://codereview.chromium.org/2977863002/diff/40001/milo/common/error.go
File milo/common/error.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/common/error.go#ne...
milo/common/error.go:25: // html frontends know how to render the error in a way
that will help the user.
html and grpc

https://codereview.chromium.org/2977863002/diff/40001/milo/common/error.go#ne...
milo/common/error.go:32: // CodeUnauthorized occurs when the user may be able to
see build, but they're
but they weren't logged in...

https://codereview.chromium.org/2977863002/diff/40001/milo/common/error.go#ne...
milo/common/error.go:36: // CodeNoAccess occurs when the user is not able to see
the build given the
was not able to see... (I noticed the convention here seemed to be, the error is
present tense, user action is past tense, just keep it consistent).

https://codereview.chromium.org/2977863002/diff/40001/milo/frontend/routes.go
File milo/frontend/routes.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/frontend/routes.go...
milo/frontend/routes.go:61: r.GET(swarming.URLBase+"/:id/steps/*logname",
basemw, func(c *router.Context) {
I'm seeing some multi-line repetition, consider moving the router Handler
function into the view_blah.go files to keep the router code smaller.  Keeping
routes.go less cluttered would be good.

[iannucci, 2017-07-14 19:00:23]

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
File milo/buildsource/buildbot/build.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbot/build.go:675: // BuildID is buildbots's notion of a
Build. See buildsource.ID.
On 2017/07/13 22:00:46, Ryan Tseng wrote:
> Make a note of why there is an error in here. (Because we validate on
> generation, but return the error only when Get() is called)
> 
> It does seem a little awkward though, I have a very slight preference towards
> making Number an string just so we don't have to carry the error around.

Done.

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
File milo/buildsource/buildbot/builder.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbot/builder.go:228: ).Tag(common.CodeParameterError).Err()
On 2017/07/13 22:00:46, Ryan Tseng wrote:
> 404?

oh yeah

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
File milo/buildsource/buildbucket/builder.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbucket/builder.go:311: // TODO(iannucci): set buildbucket
host?
On 2017/07/13 22:00:46, Ryan Tseng wrote:
> buildbucket:milo is a 1:1 relationship atm, so it's not needed currently.

yeah hence todo :)

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/buildb...
milo/buildsource/buildbucket/builder.go:312: var query builderQuery
On 2017/07/13 22:00:46, Ryan Tseng wrote:
> nit: inline

Done.

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/builde...
File milo/buildsource/builder.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/builde...
milo/buildsource/builder.go:28: func parseTwo(path, nameType, nameA, nameB
string) (a, b string, err error) {
On 2017/07/13 22:00:46, Ryan Tseng wrote:
> docstring - what is path? what is nameType? nameA? nameB? what does this
return?
> What is it for?
> 
> Actually this looks like a glorified SplitN(..., 2).  I think it's better to
get
> rid of this and do everything inline in Get(), or create another internal
method
> func (b BuilderID) Parse() (moduleName, builderGroupName, builderName string)
> {...} would be a better idea.  Becaue:
> 1. The existence of parseTwo is confusing.  The function signature doesn't
lead
> it to imply that it is good at any particular job.
> 2. The error message are confusing.  This would say "bad BuilderID: cannot
parse
> source token path" when in reality what we want to say is "bad BuilderID: path
> has less than 3 components", or "badBuilderID: Missing builder name"
> 
> I think this would be much simpler as a:
> func (b builderID) parse() (moduleName, builderGroupName, builderName string)
> {...}
> 
> your choice though

Refactored

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/swarmi...
File milo/buildsource/swarming/build.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/buildsource/swarmi...
milo/buildsource/swarming/build.go:823: func (b *BuildID) Get(ctx
context.Context) (*resp.MiloBuild, error) {
On 2017/07/13 22:00:46, Ryan Tseng wrote:
> nit: conventionally Milo uses "ctx" for router.Context and "c" for
> context.Context.

this is all goofy everywhere for reasons, but I'll try to match milo style here
:)

https://codereview.chromium.org/2977863002/diff/40001/milo/common/error.go
File milo/common/error.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/common/error.go#ne...
milo/common/error.go:25: // html frontends know how to render the error in a way
that will help the user.
On 2017/07/13 22:00:46, Ryan Tseng wrote:
> html and grpc

Done.

https://codereview.chromium.org/2977863002/diff/40001/milo/common/error.go#ne...
milo/common/error.go:32: // CodeUnauthorized occurs when the user may be able to
see build, but they're
On 2017/07/13 22:00:47, Ryan Tseng wrote:
> but they weren't logged in...

Done.

https://codereview.chromium.org/2977863002/diff/40001/milo/common/error.go#ne...
milo/common/error.go:36: // CodeNoAccess occurs when the user is not able to see
the build given the
On 2017/07/13 22:00:47, Ryan Tseng wrote:
> was not able to see... (I noticed the convention here seemed to be, the error
is
> present tense, user action is past tense, just keep it consistent).

tweaked

https://codereview.chromium.org/2977863002/diff/40001/milo/frontend/routes.go
File milo/frontend/routes.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/frontend/routes.go...
milo/frontend/routes.go:61: r.GET(swarming.URLBase+"/:id/steps/*logname",
basemw, func(c *router.Context) {
On 2017/07/13 22:00:47, Ryan Tseng wrote:
> I'm seeing some multi-line repetition, consider moving the router Handler
> function into the view_blah.go files to keep the router code smaller.  Keeping
> routes.go less cluttered would be good.

I considered that, but I prefer this way, because it keeps the route parameters
and their usage in the same place. Otherwise you have :foo here, and then
Params.ByName("foo") in some other file. Alternately, we should break the routes
themselves into the view_ files.

[Ryan Tseng, 2017-07-14 19:18:21]

lgtm

https://codereview.chromium.org/2977863002/diff/40001/milo/frontend/routes.go
File milo/frontend/routes.go (right):

https://codereview.chromium.org/2977863002/diff/40001/milo/frontend/routes.go...
milo/frontend/routes.go:61: r.GET(swarming.URLBase+"/:id/steps/*logname",
basemw, func(c *router.Context) {
On 2017/07/14 19:00:23, iannucci wrote:
> On 2017/07/13 22:00:47, Ryan Tseng wrote:
> > I'm seeing some multi-line repetition, consider moving the router Handler
> > function into the view_blah.go files to keep the router code smaller. 
Keeping
> > routes.go less cluttered would be good.
> 
> I considered that, but I prefer this way, because it keeps the route
parameters
> and their usage in the same place. Otherwise you have :foo here, and then
> Params.ByName("foo") in some other file. Alternately, we should break the
routes
> themselves into the view_ files.

makes sense then.  this is fine then.  moving them subroutes into their own
files would make this nice and small, but then we don't have all the routes in
one file.  it's a toss-up for me too.

[iannucci, 2017-07-14 19:18:51]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2017-07-14 19:19:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-14 19:24:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-14 19:24:51]

Try jobs failed on following builders:
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/375b5bddbb38de10)

[iannucci, 2017-07-14 19:31:58]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-07-14 19:31:59]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com
Link to the patchset: https://codereview.chromium.org/2977863002/#ps80001
(title: "now with case insensitivity")

[commit-bot: I haz the power, 2017-07-14 19:32:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-14 19:38:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500060718208580,
"parent_rev": "4bfe11ebfbb203db86104241a7a748df16f507ef", "commit_rev":
"3fa2a52a950e10b4eaac37b839966dd1874f5777"}

[commit-bot: I haz the power, 2017-07-14 19:38:33]

Description was changed from

==========
[milo] Refactor all html knowledge out of backends.

R=hinoka@chromium.org, estaab@chromium.org
BUG=
==========

to

==========
[milo] Refactor all html knowledge out of backends.

R=hinoka@chromium.org, estaab@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2977863002
Committed:
https://github.com/luci/luci-go/commit/3fa2a52a950e10b4eaac37b839966dd1874f5777
==========

[commit-bot: I haz the power, 2017-07-14 19:38:34]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/3fa2a52a950e10b4eaac37b839966dd1874f5777