parse full duration for time formatting #1520

jbpratt · 2022-03-04T15:54:03Z

Changes

Use native .String() impl for time.Duration

fixes #1495

Signed-off-by: jbpratt jbpratt78@gmail.com

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Run the code checkers with make check
Regenerate the manpages, docs and go formatting with make generated
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Print more accurate duration

tekton-robot · 2022-03-04T15:54:18Z

Hi @jbpratt. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jbpratt · 2022-03-04T15:56:10Z

Hi, this may not be the desired result for describing the duration, if so we can close this and close up #1495 as won't fix 😺

pkg/formatted/time_test.go

jbpratt · 2022-03-04T16:03:20Z

I think the output in question (in the issue) comes from here

cli/pkg/pipelinerun/description/description.go

Lines 113 to 119 in d1bbad0

    
           {{decorate "taskruns" ""}}{{decorate "underline bold" "Taskruns\n"}} 
        
            NAME	TASK NAME	STARTED	DURATION	STATUS 
        
           {{- range $taskrun := .TaskrunList }}{{ if checkTRStatus $taskrun }} 
        
            {{decorate "bullet" $taskrun.TaskrunName }}	{{ $taskrun.PipelineTaskName }}	{{ formatAge $taskrun.Status.StartTime $.Params.Time }}	{{ formatDuration $taskrun.Status.StartTime $taskrun.Status.CompletionTime }}	{{ formatCondition $taskrun.Status.Conditions }} 
        
           {{- end }} 
        
           {{- end }} 
        
           {{- end }}

which uses this function

cli/pkg/pipelinerun/description/description.go

Line 192 in d1bbad0

"formatDuration": formatted.Duration,

leading to this

cli/pkg/formatted/time.go

Lines 32 to 39 in d1bbad0

    
           func Duration(t1, t2 *metav1.Time) string { 
        
           	if t1.IsZero() || t2.IsZero() { 
        
           		return "---" 
        
           	} 
        
           	dur := t2.Time.Sub(t1.Time) 
        
           	return durafmt.ParseShort(dur).String() 
        
           }

Changing ParseShort to Parse results in the seemingly desired output.

jbpratt · 2022-03-05T00:42:27Z

Adding some output examples:

❯ tkn taskrun list
NAME                                                            STARTED      DURATION     STATUS
zzzzz-6xz4m                                                     1 hour ago   1 hour       Succeeded
...

❯ ./bin/tkn taskrun list
NAME                                                            STARTED      DURATION                      STATUS
zzzzz-6xz4m                                                     1 hour ago   1 hour 30 minutes 6 seconds   Succeeded
...

❯ tkn taskrun describe zzzzz-6xz4m
...
STARTED      DURATION    STATUS
1 hour ago   1 hour      Succeeded
...

❯ ./bin/tkn taskrun describe zzzzz-6xz4m
...
STARTED      DURATION                      STATUS
1 hour ago   1 hour 30 minutes 6 seconds   Succeeded

which I think is closer to what is shown in the dashboard

pradeepitm12 · 2022-03-08T07:49:06Z

/ok-to-test

tekton-robot · 2022-03-08T07:51:55Z

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/formatted/time.go	27.3%	63.6%	36.4

jbpratt · 2022-03-08T10:16:08Z

/test pull-tekton-cli-unit-tests

piyush-garg · 2022-03-08T11:15:17Z

Instead of having this long text 1 hour 30 minutes 6 seconds, I think we can have something like 1h30m6s

jbpratt · 2022-03-08T11:19:11Z

Instead of having this long text 1 hour 30 minutes 6 seconds, I think we can have something like 1h30m6s

Sure, I'll need to figure that out because the library used doesn't support that https://github.com/hako/durafmt (maybe a custom format, or this just works with native time/duration). Any suggestions are welcome :)

jbpratt · 2022-03-08T11:26:41Z

@piyush-garg PTAL, I've updated to use the native .String impl for time.Duration but days are represented differently.

❯ ./bin/tkn taskrun describe zzzzz-6xz4m
Name:              zzzzz-6xz4m
Namespace:         default
Service Account:   default
Timeout:           2h0m0s
Labels:
 app.kubernetes.io/managed-by=tekton-pipelines

🌡️  Status

STARTED      DURATION    STATUS
3 days ago   1h30m6s     Succeeded

🦶 Steps

 NAME                STATUS
 ∙ sleep-long-time   Completed

tekton-robot · 2022-03-08T11:29:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/formatted/time.go	27.3%	63.6%	36.4

Use native `time.Duration.String()` format fixes tektoncd#1495 Signed-off-by: jbpratt <jbpratt78@gmail.com>

jbpratt · 2022-03-08T11:37:22Z

Again, if this isn't desired we can close this out and leave an explanation on the ticket (#1495)

tekton-robot · 2022-03-08T11:38:56Z

The following is the coverage report on the affected files.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/formatted/time.go	27.3%	63.6%	36.4

jbpratt · 2022-03-14T11:25:46Z

Hi @pradeepitm12 @piyush-garg @vdemeester please take a look

piyush-garg · 2022-03-15T10:30:36Z

/lgtm
/cc @vdemeester @pradeepitm12 @chmouel for opinions/reviews

tekton-robot · 2022-03-15T10:30:39Z

@piyush-garg: GitHub didn't allow me to request PR reviews from the following users: for, opinions/reviews.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/lgtm
/cc @vdemeester @pradeepitm12 @chmouel for opinions/reviews

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pradeepitm12 · 2022-03-15T11:57:24Z

/lgtm

chmouel

This looks way much better to me, those "human duration" always bugged me. Feel free to clean this go.mod entry then and that should probably good to go.

pkg/formatted/time.go

chmouel · 2022-03-15T15:57:33Z

/lgtm
/approve

tekton-robot · 2022-03-15T15:57:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chmouel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [chmouel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 4, 2022

tekton-robot requested review from hrishin and sthaha March 4, 2022 15:54

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 4, 2022

jbpratt commented Mar 4, 2022

View reviewed changes

pkg/formatted/time_test.go Show resolved Hide resolved

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 8, 2022

jbpratt force-pushed the parse-duration-proper branch from 6f6dce5 to 493fbb7 Compare March 8, 2022 11:26

parse full duration for time formatting

a79baae

Use native `time.Duration.String()` format fixes tektoncd#1495 Signed-off-by: jbpratt <jbpratt78@gmail.com>

jbpratt force-pushed the parse-duration-proper branch from 493fbb7 to a79baae Compare March 8, 2022 11:36

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2022

tekton-robot requested a review from vdemeester March 15, 2022 10:30

tekton-robot assigned piyush-garg Mar 15, 2022

tekton-robot requested a review from pradeepitm12 March 15, 2022 10:30

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2022

tekton-robot requested a review from chmouel March 15, 2022 10:30

tekton-robot assigned pradeepitm12 Mar 15, 2022

chmouel reviewed Mar 15, 2022

View reviewed changes

pkg/formatted/time.go Show resolved Hide resolved

tekton-robot assigned chmouel Mar 15, 2022

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2022

tekton-robot merged commit df0da5d into tektoncd:main Mar 15, 2022

jbpratt deleted the parse-duration-proper branch March 15, 2022 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parse full duration for time formatting #1520

parse full duration for time formatting #1520

jbpratt commented Mar 4, 2022 •

edited

Loading

tekton-robot commented Mar 4, 2022

jbpratt commented Mar 4, 2022

jbpratt commented Mar 4, 2022

jbpratt commented Mar 5, 2022 •

edited

Loading

pradeepitm12 commented Mar 8, 2022

tekton-robot commented Mar 8, 2022

jbpratt commented Mar 8, 2022

piyush-garg commented Mar 8, 2022

jbpratt commented Mar 8, 2022

jbpratt commented Mar 8, 2022 •

edited

Loading

tekton-robot commented Mar 8, 2022

jbpratt commented Mar 8, 2022

tekton-robot commented Mar 8, 2022

jbpratt commented Mar 14, 2022

piyush-garg commented Mar 15, 2022

tekton-robot commented Mar 15, 2022

pradeepitm12 commented Mar 15, 2022

chmouel left a comment

chmouel commented Mar 15, 2022

tekton-robot commented Mar 15, 2022

parse full duration for time formatting #1520

parse full duration for time formatting #1520

Conversation

jbpratt commented Mar 4, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 4, 2022

jbpratt commented Mar 4, 2022

jbpratt commented Mar 4, 2022

jbpratt commented Mar 5, 2022 • edited Loading

pradeepitm12 commented Mar 8, 2022

tekton-robot commented Mar 8, 2022

jbpratt commented Mar 8, 2022

piyush-garg commented Mar 8, 2022

jbpratt commented Mar 8, 2022

jbpratt commented Mar 8, 2022 • edited Loading

tekton-robot commented Mar 8, 2022

jbpratt commented Mar 8, 2022

tekton-robot commented Mar 8, 2022

jbpratt commented Mar 14, 2022

piyush-garg commented Mar 15, 2022

tekton-robot commented Mar 15, 2022

pradeepitm12 commented Mar 15, 2022

chmouel left a comment

Choose a reason for hiding this comment

chmouel commented Mar 15, 2022

tekton-robot commented Mar 15, 2022

jbpratt commented Mar 4, 2022 •

edited

Loading

jbpratt commented Mar 5, 2022 •

edited

Loading

jbpratt commented Mar 8, 2022 •

edited

Loading