Skip to content

Show Status of Steps with TaskRun Describe#450

Merged
tekton-robot merged 1 commit intotektoncd:masterfrom
danielhelfand:tr_step_status
Nov 13, 2019
Merged

Show Status of Steps with TaskRun Describe#450
tekton-robot merged 1 commit intotektoncd:masterfrom
danielhelfand:tr_step_status

Conversation

@danielhelfand
Copy link
Copy Markdown
Member

@danielhelfand danielhelfand commented Nov 11, 2019 •
edited
Loading

Closes #440

This pull request adds the ability to see the status of a step with tkn tr describe.

Possible values are Completed and Error for a step from Terminated. Completed does not always imply the step ran successfully. It represents that no error occurred for the step, or the step never ran due to a previous step failure.

PodInitializing will appear when the step is in a Waiting state.

Running will appear when the step is actually executing and also when the pod has been initialized for the step. I am open to suggestions on if there's a better message that could be displayed to end users.

STATUS is mapped to the property Reason on the StepState from Terminated or Waiting, but will be Running if the Running property on the StepState isn't nil.

Limitation of approach in this pr is that steps are not ordered by start time. It may take some planning around how to deal with StartedAt not always being present for a step.

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)
  • Includes docs (if user facing)
  • Regenerate the manpages and docs with make docs and make man if needed.
  • Run the code checkers with make check
  • Commit messages follow commit message best practices

Release Notes

Show step status with taskrun describe

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 11, 2019
@tekton-robot
Copy link
Copy Markdown
Contributor

tekton-robot commented Nov 11, 2019

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

File Old Coverage New Coverage Delta
pkg/helper/validate/validate.go 90.0% 69.2% -20.8

@tekton-robot
Copy link
Copy Markdown
Contributor

tekton-robot commented Nov 11, 2019

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

File Old Coverage New Coverage Delta
pkg/helper/validate/validate.go 90.0% 92.3% 2.3

piyush-garg
piyush-garg approved these changes Nov 11, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@piyush-garg piyush-garg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2019
@danielhelfand danielhelfand changed the title Show Status of Steps with TaskRun Describe WIP: Show Status of Steps with TaskRun Describe Nov 11, 2019
@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2019
@danielhelfand
Copy link
Copy Markdown
Member Author

danielhelfand commented Nov 11, 2019

Changing to WIP as I am now seeing Waiting and Running aspects of StepState that should be taken into account.

@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2019
@tekton-robot
Copy link
Copy Markdown
Contributor

tekton-robot commented Nov 11, 2019

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

File Old Coverage New Coverage Delta
pkg/helper/validate/validate.go 90.0% 94.1% 4.1

@danielhelfand danielhelfand changed the title WIP: Show Status of Steps with TaskRun Describe Show Status of Steps with TaskRun Describe Nov 11, 2019
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2019
@danielhelfand
Copy link
Copy Markdown
Member Author

danielhelfand commented Nov 11, 2019

/test pull-tekton-cli-unit-tests

danielhelfand
danielhelfand commented Nov 11, 2019
View reviewed changes
Comment thread pkg/helper/validate/validate.go Outdated
Copy link
Copy Markdown
Member Author

@danielhelfand danielhelfand Nov 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions is there's a better message that can be returned here. The issue centers around that this will display when the step is not actually executing. state.Running != nil when the pod is initialized and container is ready.

Copy link
Copy Markdown
Member

@vdemeester vdemeester Nov 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielhelfand yeah for now this is the best we can do I think…

@danielhelfand
Copy link
Copy Markdown
Member Author

danielhelfand commented Nov 11, 2019 •
edited
Loading

I have updated the pr description and have updated the pr to take into account Running and Waiting for the StepStatus. This is ready for another review.

@tekton-robot
Copy link
Copy Markdown
Contributor

tekton-robot commented Nov 11, 2019

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

File Old Coverage New Coverage Delta
pkg/helper/validate/validate.go 90.0% 94.1% 4.1

vdemeester
vdemeester approved these changes Nov 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Comment thread pkg/helper/validate/validate.go Outdated
Copy link
Copy Markdown
Member

@vdemeester vdemeester Nov 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielhelfand yeah for now this is the best we can do I think…

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2019
@tekton-robot
Copy link
Copy Markdown
Contributor

tekton-robot commented Nov 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: piyush-garg, vdemeester

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2019
@tekton-robot tekton-robot merged commit f47dc4c into tektoncd:master Nov 13, 2019
@danielhelfand danielhelfand mentioned this pull request Nov 13, 2019
Closed
chmouel added a commit that referenced this pull request Nov 22, 2019
@chmouel
New version v0.5.1
13bf66a 
#445 | [Chmouel Boudjnah] Fix brew install from tap | 2019/11/08-05:20
#446 | [Chmouel Boudjnah] Update README to 0.5.0 | 2019/11/08-07:01
#443 | [Piyush Garg] Show param default value in task describe | 2019/11/08-07:45
#443 | [Piyush Garg] Change DEFAULT_VALUE to DEFAULT VALUE | 2019/11/08-07:45
#447 | [Chmouel Boudjnah] Add Release process documentation | 2019/11/08-08:23
#449 | [Vincent Demeester] Linting yamls with yamllint 🏷 | 2019/11/08-10:09
#452 | [Piyush Garg] Add cloudEvent resource support to start command | 2019/11/11-22:56
#457 | [Piyush Garg] Fix resource creation failure in case of multiple | 2019/11/13-00:49
#450 | [danielhelfand] show status of steps with taskrun describe | 2019/11/13-08:00
#461 | [Piyush Garg] Update go.sum | 2019/11/14-03:01
null | [Vincent Demeester] Bump plumbing to latest changes 🔗 | 2019/11/14-04:36
null | [Dan Lorenc] Add a --filename parameter to 'tkn tasks start'. | 2019/11/15-02:38
null | [Piyush Garg] Fix interactive input params on pipeline start | 2019/11/15-03:54
null | [Daniel Helfand] include os and version in issue template | 2019/11/18-14:57
null | [Chmouel Boudjnah] Add Installtion instruction when compiling from source | 2019/11/20-06:23
null | [Navid Shaikh] Uses vendored deps while building the binary in Makefile | 2019/11/20-07:58
null | [Daniel Helfand] add check in pr logs for if condition available | 2019/11/21-02:53

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdemeester vdemeester vdemeester approved these changes

@sthaha sthaha Awaiting requested review from sthaha

+1 more reviewer

@piyush-garg piyush-garg piyush-garg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vdemeester vdemeester

@piyush-garg piyush-garg

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Show Step Status for TaskRun Describe

5 participants

@danielhelfand @tekton-robot @vdemeester @piyush-garg @googlebot