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.

Sign up for GitHub

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

Jump to bottom

Add --all-namespaces --no-headers flags to task and pipeline list subcommand #840

Merged
merged 3 commits into from
Apr 17, 2020
Merged

Add --all-namespaces --no-headers flags to task and pipeline list subcommand #840

merged 3 commits into from
Apr 17, 2020

Conversation

waveywaves
Copy link
Member

@waveywaves waveywaves commented Mar 27, 2020 •
edited
Loading

Signed-off-by: Vibhav Bobade vibhav.bobde@gmail.com

Changes

Add flags for task and pipeline list subcommand

  • task list --all-namespaces
  • task list --no-headers
  • pipeline list --all-namespaces
  • pipeline list --no-headers

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

release-note

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 27, 2020
@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch 3 times, most recently from 6d93934 to 9b6ab0f Compare March 27, 2020 23:29
@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch 3 times, most recently from 4511c2f to 14bbb81 Compare March 28, 2020 22:45
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 28, 2020
@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch 2 times, most recently from 03b88b9 to 9ccc86f Compare March 28, 2020 23:12
@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch from 9ccc86f to 77c8047 Compare March 29, 2020 01:44
@waveywaves waveywaves mentioned this pull request Mar 29, 2020
Closed
@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch 5 times, most recently from d07f058 to ed18f5b Compare March 29, 2020 03:32
@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

5 similar comments
@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests

@waveywaves
Copy link
Member Author

waveywaves commented Apr 12, 2020 •
edited
Loading

@danielhelfand Based on my last comment

ns in this case is being used as a place holder after testing a bit. I removed it, and also tried keeping it empty, but got the error Error: namespaces "" not found. And adding more
"namespaces" to ns we get the error Error: more than one object matched gvr /v1, Resource=namespaces, ns: "" name: ""

I feel this seems like a bug in testing. As certain aspects of seeddata generation in unit testing are flawed, with us not able to specify namespaces which should be present for a unit test. The ns with one namespace specified in unit tests here as we have seen is definitely a placeholder and the purpose of it is unknown. Maybe opening an issue would make sense for this.

@danielhelfand
Copy link
Member

danielhelfand commented Apr 12, 2020 •
edited
Loading

@danielhelfand Based on my last comment

ns in this case is being used as a place holder after testing a bit. I removed it, and also tried keeping it empty, but got the error Error: namespaces "" not found. And adding more
"namespaces" to ns we get the error Error: more than one object matched gvr /v1, Resource=namespaces, ns: "" name: ""

I feel this seems like a bug in testing. As certain aspects of seeddata generation in unit testing are flawed, with us not able to specify namespaces which should be present for a unit test. The ns with one namespace specified in unit tests here as we have seen is definitely a placeholder and the purpose of it is unknown. Maybe opening an issue would make sense for this.

A solution may be to avoid checking for a namespace if -A is specified. So don't perform this check if -A is true:

cli/pkg/cmd/task/list.go

Line 51 in 4450ca5

if err := validate.NamespaceExists(p); err != nil {

@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch from 9a4ebec to 04da808 Compare April 12, 2020 19:48
@waveywaves
Copy link
Member Author

@danielhelfand You were right, updating namespace checking condition as you said did work ! 🤗 I spent quite some time before trying to solve for this error, and stumbled on kube testing source 🤕 .

@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch from 04da808 to 18d6406 Compare April 12, 2020 20:05
@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch from 18d6406 to c33031a Compare April 13, 2020 08:23
danielhelfand
danielhelfand reviewed Apr 13, 2020
View reviewed changes
Copy link
Member

@danielhelfand danielhelfand 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 Apr 13, 2020
pradeepitm12
pradeepitm12 reviewed Apr 15, 2020
View reviewed changes
pkg/cmd/task/list.go Outdated Show resolved Hide resolved
pradeepitm12
pradeepitm12 reviewed Apr 15, 2020
View reviewed changes
pkg/cmd/task/list.go Outdated Show resolved Hide resolved
@waveywaves
Add --no-headers and --all-namespaces flag for Task list subcommand
2926cce 
--no-headers flag helps omit the headers or the index
row of the output while printing. This allows the user
to parse the output without having to deal with the
headers in the output.
--all-namespaces flag helps to print Tasks from all
namespaces. These tasks are grouped by namespace
and then sorted based on StartTime
@waveywaves
Add --no-headers and --all-namespaces flag for Pipeline list
2649b31 
--no-headers flag helps omit the headers or the index
row of the output while printing. This allows the user
to parse the output without having to deal with the
headers in the output.
--all-namespaces flag helps to print Pipelines from all
namespaces. These tasks are grouped by namespace
and then sorted based on StartTime.
@waveywaves
Docs --no-headers --all-namespaces flags for task/pipeline list
aecdac3 
Add man page and docs for the two flags
@waveywaves waveywaves force-pushed the 785-list-all-namespaces-task-pipeline branch from c33031a to aecdac3 Compare April 17, 2020 08:36
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@waveywaves
Copy link
Member Author

@pradeepitm12 Can you PTAL ?

danielhelfand
danielhelfand approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@danielhelfand danielhelfand left a comment

Choose a reason for hiding this comment

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

cc @pradeepitm12

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielhelfand

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 17, 2020
@pradeepitm12
Copy link
Contributor

@waveywaves Thanks for addressing all the reviews.

pradeepitm12
pradeepitm12 reviewed Apr 17, 2020
View reviewed changes
Copy link
Contributor

@pradeepitm12 pradeepitm12 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 Apr 17, 2020
@danielhelfand
Copy link
Member

The failure here does seem related:

I0417 16:34:10.074]     TestPipelinesNegativeE2E/Get_list_of_Pipelines_from_namespace__testcli-shsbb: pipeline_e2e_test.go:348: assertion failed: 
I0417 16:34:10.074]         Command:  /go/src/github.com/tektoncd/cli/tkn pipelines list -n testcli-shsbb
I0417 16:34:10.078]         ExitCode: 0
I0417 16:34:10.078]         Stdout:   NAME              AGE            LAST RUN   STARTED   DURATION   STATUS
I0417 16:34:10.078]         output-pipeline   1 second ago   ---        ---       ---        ---
I0417 16:34:10.078]         
I0417 16:34:10.079]         Stderr:   
I0417 16:34:10.079]         
I0417 16:34:10.079]         Failures:
I0417 16:34:10.079]         Expected stdout to contain "NAME              AGE             LAST RUN   STARTED   DURATION   STATUS\noutput-pipeline   2 seconds ago   ---        ---       ---        ---\n"

@waveywaves
Copy link
Member Author

/test pull-tekton-cli-integration-tests-0_10

@waveywaves
Copy link
Member Author

@danielhelfand Seems like a flake. The only change is in the age of the pipeline

@tekton-robot tekton-robot merged commit 2d9aad8 into tektoncd:master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pradeepitm12 pradeepitm12 pradeepitm12 left review comments

@danielhelfand danielhelfand danielhelfand approved these changes

@chmouel chmouel Awaiting requested review from chmouel

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees

@pradeepitm12 pradeepitm12

@danielhelfand danielhelfand

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@waveywaves @danielhelfand @tekton-robot @pradeepitm12 @googlebot