milo: fix running steps

milo: fix running steps 

milo displays running steps as succeeded because annotation processor incorrectly closes all steps in the end. Fix that.

Milo did not have css rule for running builds. Fix that.

Also do not add empty property groups to milo build.

Also fix build duration for running builds.

Refactor some code related to changes above.

R=hinoka@chromium.org, estaab@chromium.org, dnj@chromium.org
BUG=613584
Committed: https://github.com/luci/luci-go/commit/30e6ec55c034c0c327a38c25a040727a07276c61

[nodir, 2016-06-16 19:36:07]

Description was changed from

==========
milo: fix running steps

annotation processor incorrectly closed all steps in the end. Do not do
that.

Milo did not have css rule for running builds. Add that.

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

to

==========
milo: fix running steps 

milo displays running steps as succeeded because annotation processor
incorrectly closes all steps in the end. Fix that.

Milo did not have css rule for running builds. Fix that.

Also do not add empty property groups to milo build.

Also fix build duration for running builds.

Refactor some code related to changes above.

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

[nodir, 2016-06-16 19:44:33]

Patchset #2 (id:20001) has been deleted

[nodir, 2016-06-16 19:44:45]

Patchset #1 (id:1) has been deleted

[nodir, 2016-06-16 20:01:49]

PTAL

Screenshot: http://i.imgur.com/sZ9fbsB.png
- "recipe bootstrap" and "go third parties" steps are yellow and have durations
- No build end time, but has "Elapsed"

[nodir, 2016-06-16 20:02:00]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 20:02:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078603002/60001

[estaab, 2016-06-16 20:13:45]

lgtm

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/fron...
File appengine/cmd/milo/frontend/static/buildbot/css/default.css (right):

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/fron...
appengine/cmd/milo/frontend/static/buildbot/css/default.css:435:
background-color: #fd3;
#fff6c6 as per buildbot?

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/build.go (right):

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:257: if t == "" {
What does an empty tag mean?

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:275: case TaskRunning:
nice, much better with these constants.

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:312: // timezone information,. Make them
valid RFC3339Nano.
extra comma

[commit-bot: I haz the power, 2016-06-16 20:18:34]

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

[commit-bot: I haz the power, 2016-06-16 20:18:35]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2016-06-16 20:21:12]

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

[Ryan Tseng, 2016-06-16 20:21:13]

lgtm

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/fron...
File appengine/cmd/milo/frontend/static/buildbot/css/default.css (right):

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/fron...
appengine/cmd/milo/frontend/static/buildbot/css/default.css:475:
.running,.waiting,td.building {
Just use this

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/build.go (right):

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:300: return nil, fmt.Errorf("unknown task
state %q", sr.State)
unknown swarming task state

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:304: if props := taskProperties(sr);
len(props.Property) > 0 {
nit: Not actually sure if the if statement are necessary...?

https://codereview.chromium.org/2078603002/diff/60001/client/logdog/annotee/p...
File client/logdog/annotee/processor.go (right):

https://codereview.chromium.org/2078603002/diff/60001/client/logdog/annotee/p...
client/logdog/annotee/processor.go:259: func (p *Processor) Finish(closeSteps
bool) *annotation.State {
Shouldn't this be in p.o.closeSteps already?

https://codereview.chromium.org/2078603002/diff/60001/common/proto/milo/annot...
File common/proto/milo/annotations.proto (right):

https://codereview.chromium.org/2078603002/diff/60001/common/proto/milo/annot...
common/proto/milo/annotations.proto:1: // Copyright (c) 2015 The LUCI Authors.
All rights reserved.
run "go generate" here.  (Pretty sure comments are carried over)

[Ryan Tseng, 2016-06-16 20:22:37]

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/fron...
File appengine/cmd/milo/frontend/static/buildbot/css/default.css (right):

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/fron...
appengine/cmd/milo/frontend/static/buildbot/css/default.css:435:
background-color: #fd3;
On 2016/06/16 20:13:45, estaab wrote:
> #fff6c6 as per buildbot?

old (our version) buildbot uses fff6c6, new(er) versions of buildbot uses fd3. 
I'm impartial to which one we use, but I think i stole^W borrowed this
default.css from the most recent version of buildbot.

[nodir, 2016-06-16 21:05:15]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 21:05:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078603002/80001

[nodir, 2016-06-16 21:08:05]

Looks ok:
https://879-3f5844f-tainted-nodir-dot-luci-milo.appspot.com/swarming/prod/2f7...

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/fron...
File appengine/cmd/milo/frontend/static/buildbot/css/default.css (right):

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/fron...
appengine/cmd/milo/frontend/static/buildbot/css/default.css:475:
.running,.waiting,td.building {
On 2016/06/16 20:21:13, Ryan Tseng wrote:
> Just use this

Done.

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/build.go (right):

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:257: if t == "" {
On 2016/06/16 20:13:45, estaab wrote:
> What does an empty tag mean?

we should not have empty tags, but without this if statement the parts[0] below
will panic

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:304: if props := taskProperties(sr);
len(props.Property) > 0 {
On 2016/06/16 20:21:13, Ryan Tseng wrote:
> nit: Not actually sure if the if statement are necessary...?

it avoids adding property groups that don't have properties, e.g. if there is no
task code and exit code, or if there are no tags

https://codereview.chromium.org/2078603002/diff/60001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:312: // timezone information,. Make them
valid RFC3339Nano.
On 2016/06/16 20:13:45, estaab wrote:
> extra comma

Done.

https://codereview.chromium.org/2078603002/diff/60001/client/logdog/annotee/p...
File client/logdog/annotee/processor.go (right):

https://codereview.chromium.org/2078603002/diff/60001/client/logdog/annotee/p...
client/logdog/annotee/processor.go:259: func (p *Processor) Finish(closeSteps
bool) *annotation.State {
On 2016/06/16 20:21:13, Ryan Tseng wrote:
> Shouldn't this be in p.o.closeSteps already?

good catch, done.

https://codereview.chromium.org/2078603002/diff/60001/common/proto/milo/annot...
File common/proto/milo/annotations.proto (right):

https://codereview.chromium.org/2078603002/diff/60001/common/proto/milo/annot...
common/proto/milo/annotations.proto:1: // Copyright (c) 2015 The LUCI Authors.
All rights reserved.
On 2016/06/16 20:21:13, Ryan Tseng wrote:
> run "go generate" here.  (Pretty sure comments are carried over)

Done.

[hinoka, 2016-06-16 21:13:59]

https://codereview.chromium.org/2078603002/diff/80001/client/logdog/annotee/p...
File client/logdog/annotee/processor.go (right):

https://codereview.chromium.org/2078603002/diff/80001/client/logdog/annotee/p...
client/logdog/annotee/processor.go:301: h.finish(closeSteps)
I don' think you have to pass it in here either

[commit-bot: I haz the power, 2016-06-16 21:16:13]

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

[commit-bot: I haz the power, 2016-06-16 21:16:15]

Dry run: This issue passed the CQ dry run.

[nodir, 2016-06-16 21:23:14]

https://codereview.chromium.org/2078603002/diff/80001/client/logdog/annotee/p...
File client/logdog/annotee/processor.go (right):

https://codereview.chromium.org/2078603002/diff/80001/client/logdog/annotee/p...
client/logdog/annotee/processor.go:301: h.finish(closeSteps)
On 2016/06/16 21:13:58, hinoka wrote:
> I don' think you have to pass it in here either

why?

https://codereview.chromium.org/2078603002/diff/80001/client/logdog/annotee/p...
client/logdog/annotee/processor.go:301: h.finish(closeSteps)
On 2016/06/16 21:13:58, hinoka wrote:
> I don' think you have to pass it in here either

it does not always equal p.o.CloseSteps, e.g. 2 hunks above

[nodir, 2016-06-16 21:23:47]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-06-16 21:23:48]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com,
estaab@chromium.org
Link to the patchset: https://codereview.chromium.org/2078603002/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2016-06-16 21:23:54]

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

[commit-bot: I haz the power, 2016-06-16 21:23:55]

This CL has an open dependency (Issue 2077593002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[nodir, 2016-06-16 21:48:31]

The CQ bit was checked by nodir@chromium.org

[commit-bot: I haz the power, 2016-06-16 21:48:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078603002/80001

[commit-bot: I haz the power, 2016-06-16 21:52:07]

Description was changed from

==========
milo: fix running steps 

milo displays running steps as succeeded because annotation processor
incorrectly closes all steps in the end. Fix that.

Milo did not have css rule for running builds. Fix that.

Also do not add empty property groups to milo build.

Also fix build duration for running builds.

Refactor some code related to changes above.

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

to

==========
milo: fix running steps 

milo displays running steps as succeeded because annotation processor
incorrectly closes all steps in the end. Fix that.

Milo did not have css rule for running builds. Fix that.

Also do not add empty property groups to milo build.

Also fix build duration for running builds.

Refactor some code related to changes above.

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

Committed:
https://github.com/luci/luci-go/commit/30e6ec55c034c0c327a38c25a040727a07276c61
==========

[commit-bot: I haz the power, 2016-06-16 21:52:09]

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