Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance the output of "kubectl get taskruns" #718

Merged
merged 1 commit into from
Apr 8, 2019

Conversation

vincent-pli
Copy link
Member

@vincent-pli vincent-pli commented Apr 4, 2019

Fix #717 #696

Changes

Any details please check the issue attached, I just want to enhance the output of "kubectl get taskrun", to make it more clear and friendly, open for discussion, thanks

Submitter Checklist

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

See the contribution guide
for more details.

Release Notes

release-note

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 4, 2019
@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 4, 2019
pkg/reconciler/v1alpha1/taskrun/taskrun.go Outdated Show resolved Hide resolved
config/300-taskrun.yaml Show resolved Hide resolved
@hrishin
Copy link
Member

hrishin commented Apr 4, 2019

@vincent-pli thanks! for the PR 👍 .

Would you mind to update kubectl get task .. section in the https://github.com/tektoncd/pipeline/blob/master/docs/tutorial.md#task-inputs-and-outputs and maybe respective doc references? 🙂

config/300-taskrun.yaml Outdated Show resolved Hide resolved
Copy link
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.

We should go for something more like knative/build, aka https://github.com/knative/build/blob/master/config/300-build.yaml
(that's my fault on the initial PR, I should have commented that out)

Copy link
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.

@vincent-pli so I would like for us to not change anything in the status and just go with what knative/build does.

  additionalPrinterColumns:
  - name: Succeeded
    type: string
    JSONPath: ".status.conditions[?(@.type==\"Succeeded\")].status"
  - name: Reason
    type: string
    JSONPath: ".status.conditions[?(@.type==\"Succeeded\")].reason"
  - name: StartTime
    type: date
    JSONPath: .status.startTime
  - name: CompletionTime
    type: date
    JSONPath: .status.completionTime

Using kubectl to see status of the pipeline/task runs is not the end goal. A cli is in discussion, a dashoard is also on the way. We should do "just" what we can with kubectl.

@vincent-pli
Copy link
Member Author

@vdemeester
Sorry to reply later, thanks for comments.
That's great if there will be a cli or dashboard to monitoring the status transformation.

For your comments, about the setting from Knative-build, that's make sense, I have tried it myself, get this:

NAME          SUCCEEDED   REASON         STARTTIME   COMPLETIONTIME
hello-build   False       BuildTimeout   10d         10d

That's looks great.
Just one thing: the "reason". Seems in pipeline, we not prepare enough explicit and meaningful expressing for it, only two: "Pending"(when pod pending) and "Building "(when pod running), even worse, at the same time, the "SUCCEDED" is "unknown", that's confuse.

I checked the code in the Knative-build, they prepare more "reasons", such as: "BuildCancelled", "BuildTimeout", "BuildValidationFailed".

So In my opinion, if we decide to leverage cli or dashboard, we just remove these information from kubectl, just like what "argo" did, or we need build a more meaningful "reason",

What do you think? :)

@vdemeester
Copy link
Member

@vincent-pli yeah we may want to add more/meaninful Reason, but it's orthogonal for this PR though 👼. Let's do "best-effort" for this PR (with #718 (review)).

So In my opinion, if we decide to leverage cli or dashboard, we just remove these information from kubectl, just like what "argo" did, or we need build a more meaningful "reason"

We'll use the information from the API (same way kubectl does) but we won't actually depend on kubectl in anyway (or it's magic printable column thing 👼 ).

@vincent-pli
Copy link
Member Author

@vdemeester
Thank for your clarification,I agree.
We could adopt the solution of Knative-build first, then prepare more meaningful "reason" later.

@vdemeester
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 8, 2019
Copy link
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.

Given that #740 is merged, CompletionTime will now be present

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester, vincent-pli

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:

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 Apr 8, 2019
@tekton-robot tekton-robot merged commit 828bcc9 into tektoncd:master Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants