Issue #1488 - added build duration and tweaked build counting in describer#1624
Issue #1488 - added build duration and tweaked build counting in describer#1624openshift-bot merged 1 commit intoopenshift:masterfrom
Conversation
|
This is how it looks with this: |
pkg/cmd/cli/describe/describer.go
Outdated
There was a problem hiding this comment.
StartTimestamp could be nil. For that matter so could CompletionTimestamp so i'm surprised this is working?
In anycase you need to look at this logic to see how complex this really needs to be do to it right (which is why I didn't do it :) )
https://github.com/openshift/origin/blob/master/pkg/cmd/cli/describe/describer.go#L114
There was a problem hiding this comment.
If so, I'll extract a method for doing that, thx.
There was a problem hiding this comment.
See pkg/api/graph/deployment.go, if you extract extract it from both places.
----- Original Message -----
for i := range sortedBuilds { // iterate backwards so we're printing the newest items first build := sortedBuilds[len(sortedBuilds)-1-i]
build.CreationTimestamp.Rfc3339Copy().Time))fmt.Fprintf(out, fmt.Sprintf("- %s %s %v\n", build.Name, build.Status,c++ build.CompletionTimestamp.Rfc3339Copy().Time.Sub(build.StartTimestamp.Rfc3339Copy().Time)duration :=If so, I'll extract a method for doing that, thx.
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1624/files#r27883923
There was a problem hiding this comment.
Also see #1621
----- Original Message -----
See pkg/api/graph/deployment.go, if you extract extract it from both places.
----- Original Message -----
for i := range sortedBuilds { // iterate backwards so we're printing the newest items first build := sortedBuilds[len(sortedBuilds)-1-i]
build.Status,fmt.Fprintf(out, fmt.Sprintf("- %s %s %v\n", build.Name,
build.CreationTimestamp.Rfc3339Copy().Time))c++ build.CompletionTimestamp.Rfc3339Copy().Time.Sub(build.StartTimestamp.Rfc3339Copy().Time)duration :=If so, I'll extract a method for doing that, thx.
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1624/files#r27883923
There was a problem hiding this comment.
That section is insane, and means the server should be returning duration so that clients don't have to implement it. :)
There was a problem hiding this comment.
Yeah, I was about to write that, after carefully studying the code you've pointed to, where I see you also print waiting time as well.
There was a problem hiding this comment.
The Duration here will specifically be of the build itself, meaning if it hasn't started yet (CreationTimestamp==0) it's Duration will be 0. In all other cases it will always show the Duration for the moment of GET, which obviously mean it'll change for running builds, but will be constant for Finished, Cancelled etc.
There was a problem hiding this comment.
that would be a change for waiting builds since today we have the duration for builds that haven't started yet...what's your plan for that?
There was a problem hiding this comment.
That waiting information is only in build describe and I'll leave it there as is, since I'm not planning to add descriptive field about the duration it'll contain just the build duration. So in a case of waiting duration will be 0.
There was a problem hiding this comment.
Something like that:
Builds:
Name Status Duration Creation Time
ruby-sample-build-23 Complete 50s 2015-04-08 13:26:53 +0000 UTC
ruby-sample-build-22 Pending 0 2015-04-08 13:18:52 +0000 UTC
ruby-sample-build-20 Failed 31s 2015-04-08 13:18:51 +0000 UTC
|
Use tab writer to align the columns - the uneven output is ugly. |
|
Lowercase the status, and use formatRelativeTime() instead of displaying the exact time. |
|
Drop the dash in front of each line and do an indent consistent with the rest of the page (usually \t but smaller is ok) |
|
I've addressed all the comments except for the |
pkg/cmd/cli/describe/describer.go
Outdated
There was a problem hiding this comment.
you're going to output "waiting for XXXs" for builds that were canceled before starting. Should output "waited"
There was a problem hiding this comment.
OK, I'll prepare test cases for that and then I'll change it accordingly.
|
one nit, otherwise this looks much nicer, thanks! |
|
@bparees addressed that one comment and added tests for that, squashed it all, ready to merge. |
|
LGTM |
|
latm [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1638/) (Image: devenv-fedora_1327) |
…g in describer
|
Rebased. |
|
re-[test] I don't understand why server died when testing UI. |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1875/) |
|
[merge] |
|
Evaluated for origin up to 1f63923 |
Merged by openshift-bot
@bparees PTAL, I've added duration and tweaked the build counting 😉 and now we can safely close #1488