Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2772623002:
Milo: Don't float step times if build has finished (Closed)
|
Created:
3 years, 9 months ago by Ryan Tseng Modified:
3 years, 8 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: Don't float step times if build has finished
BUG=690932
Review-Url: https://codereview.chromium.org/2772623002
Committed: https://github.com/luci/luci-go/commit/f842e4483134ce898e50d3387babae715ff87b24
Patch Set 1 #
Total comments: 8
Patch Set 2 : review #Patch Set 3 : Compile fix #
Messages
Total messages: 29 (21 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...
hinoka@chromium.org changed reviewers: + hinoka@chromium.org, nodir@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm note that in your demo that step is yellow, but that's another CL for crbug.com/697936 https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... File milo/appengine/buildbot/build.go (right): https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... milo/appengine/buildbot/build.go:70: // of seconds since epoch) to a natime time.Time object. native https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... milo/appengine/buildbot/build.go:73: result = time.Unix(int64(*t), int64(*t*1e9)%1e9).UTC() I think this needs gofmt https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... milo/appengine/buildbot/build.go:78: // parseTimes translates a buildbot time tuple (start/end) into a triplet nit: standard notation is that tuple item separator is comma, not slash https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... milo/appengine/buildbot/build.go:80: // If buildFinished is not nil, then all non-specified end duration will be set to it says "all" like there are multiple end duration. Also, what is "end duration"? Consider something along: If times[1] is nil and buildFinished is not, ended will be set to buildFinished time.
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/2772623002/diff/1/milo/appengine/buildbot/bui... File milo/appengine/buildbot/build.go (right): https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... milo/appengine/buildbot/build.go:70: // of seconds since epoch) to a natime time.Time object. On 2017/03/24 06:23:49, nodir wrote: > native Done. https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... milo/appengine/buildbot/build.go:73: result = time.Unix(int64(*t), int64(*t*1e9)%1e9).UTC() On 2017/03/24 06:23:49, nodir wrote: > I think this needs gofmt Just did... nothing changed. (It's also a write hook on my vim setup) https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... milo/appengine/buildbot/build.go:78: // parseTimes translates a buildbot time tuple (start/end) into a triplet On 2017/03/24 06:23:49, nodir wrote: > nit: standard notation is that tuple item separator is comma, not slash Done. https://codereview.chromium.org/2772623002/diff/1/milo/appengine/buildbot/bui... milo/appengine/buildbot/build.go:80: // If buildFinished is not nil, then all non-specified end duration will be set to On 2017/03/24 06:23:49, nodir wrote: > it says "all" like there are multiple end duration. Also, what is "end > duration"? > > Consider something along: > > If times[1] is nil and buildFinished is not, ended will be set to buildFinished > time. Done.
The CQ bit was unchecked by hinoka@chromium.org
The CQ bit was checked by hinoka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org Link to the patchset: https://codereview.chromium.org/2772623002/#ps20001 (title: "review")
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@chromium.org
The CQ bit was checked by hinoka@chromium.org
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 commit-bot@chromium.org
Try jobs failed on following builders: Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/352ef7f15c489110)
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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hinoka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org Link to the patchset: https://codereview.chromium.org/2772623002/#ps40001 (title: "Compile fix")
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": 40001, "attempt_start_ts": 1490739249077630,
"parent_rev": "e602d8d87c1091e3be20e7ddc7c229343cd84927", "commit_rev":
"f842e4483134ce898e50d3387babae715ff87b24"}
Message was sent while issue was closed.
Description was changed from ========== Milo: Don't float step times if build has finished BUG=690932 ========== to ========== Milo: Don't float step times if build has finished BUG=690932 Review-Url: https://codereview.chromium.org/2772623002 Committed: https://github.com/luci/luci-go/commit/f842e4483134ce898e50d3387babae715ff87b24 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://github.com/luci/luci-go/commit/f842e4483134ce898e50d3387babae715ff87b24 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
