Skip to content

Add flag to list pipelineruns and taskruns from all namespaces#791

Merged
tekton-robot merged 3 commits intotektoncd:masterfrom
waveywaves:785-list-all-namespaces
Mar 20, 2020
Merged

Add flag to list pipelineruns and taskruns from all namespaces#791
tekton-robot merged 3 commits intotektoncd:masterfrom
waveywaves:785-list-all-namespaces

Conversation

@waveywaves
Copy link
Copy Markdown
Member

@waveywaves waveywaves commented Mar 10, 2020

Changes

partly fixes #785

Add --all-namespaces flag to list subcommands for TaskRun and PipelineRun.
This allows the user to list taskruns and pipelineruns in all namespaces respectively.

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
Copy link
Copy Markdown
Contributor

Hi @waveywaves. 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.

Details

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.

@tekton-robot tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 10, 2020
Copy link
Copy Markdown
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.

Thanks for this! Just adding a note that this would not completely solve #785 as the issue is for every list command in the CLI. I would remove fixes before #785 and change to Part of #785.

Comment thread pkg/cmd/pipelinerun/list.go Outdated
Comment thread pkg/cmd/taskrun/list.go Outdated
Comment thread pkg/cmd/pipelinerun/list.go Outdated
@danielhelfand
Copy link
Copy Markdown
Member

/cc @pradeepitm12

Any feedback on this?

@waveywaves
Copy link
Copy Markdown
Member Author

Thanks for this! Just adding a note that this would not completely solve #785 as the issue is for every list command in the CLI. I would remove fixes before #785 and change to Part of #785.

I will update this PR with changes to all list subcommands in that case ✍️

@waveywaves
Copy link
Copy Markdown
Member Author

@danielhelfand can you /ok-to-test this ?

@danielhelfand
Copy link
Copy Markdown
Member

/ok-to-test

@tekton-robot 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 10, 2020
@waveywaves waveywaves changed the title Add flag to list pipelineruns and taskruns from all namespaces WIP : Add flag to list pipelineruns and taskruns from all namespaces Mar 10, 2020
@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 Mar 10, 2020
Copy link
Copy Markdown
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.

@waveywaves Appreciated your efforts for this PR, could you please update the PR as per the comments.

Comment thread pkg/cmd/pipelinerun/list.go Outdated
Comment thread pkg/cmd/pipelinerun/list.go Outdated
Comment thread pkg/cmd/taskrun/list.go Outdated
Comment thread docs/cmd/tkn_pipelinerun_list.md Outdated
@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch from 2e9f02d to de79f20 Compare March 11, 2020 12:18
@ppitonak
Copy link
Copy Markdown
Contributor

ppitonak commented Mar 11, 2020

Could we have the same implementation as in kubectl or oc? I.e. add also short version of this flag -A and ignore explicitly specified namespace.

-A, --all-namespaces=false: If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even if specified with --namespace.

@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch 7 times, most recently from 4e179d5 to 3296334 Compare March 11, 2020 15:01
@waveywaves
Copy link
Copy Markdown
Member Author

@ppitonak I ahve added the shorthand for all Namespaces

@waveywaves
Copy link
Copy Markdown
Member Author

The integration tests seem to be failing because what they are testing against is a string concat and not a template output

@googlebot
Copy link
Copy Markdown

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch from 322212d to c70ec29 Compare March 18, 2020 21:01
@tekton-robot
Copy link
Copy Markdown
Contributor

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/pipelinerun/sort/by_namespace.go Do not exist 100.0%
pkg/taskrun/sort/by_namespace.go Do not exist 100.0%

@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch from c70ec29 to a508362 Compare March 18, 2020 21:10
@tekton-robot
Copy link
Copy Markdown
Contributor

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/pipelinerun/sort/by_namespace.go Do not exist 100.0%
pkg/taskrun/sort/by_namespace.go Do not exist 100.0%

@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch from a508362 to 9a8f430 Compare March 18, 2020 22:38
@tekton-robot
Copy link
Copy Markdown
Contributor

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/pipelinerun/sort/by_namespace.go Do not exist 88.9%
pkg/taskrun/sort/by_namespace.go Do not exist 88.9%

@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch from 9a8f430 to fdf774d Compare March 18, 2020 23:04
@tekton-robot
Copy link
Copy Markdown
Contributor

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/pipelinerun/sort/by_namespace.go Do not exist 100.0%
pkg/taskrun/sort/by_namespace.go Do not exist 100.0%

@waveywaves
Copy link
Copy Markdown
Member Author

/test pull-tekton-cli-build-tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Keep one blank line

Comment thread pkg/taskrun/sort/by_namespace_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Keep one blank line

@piyush-garg
Copy link
Copy Markdown
Contributor

Please add license to new files and also imports in the file needs to be formatted.

@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch from fdf774d to 97628a8 Compare March 19, 2020 11:54
@tekton-robot
Copy link
Copy Markdown
Contributor

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/pipelinerun/sort/by_namespace.go Do not exist 100.0%
pkg/taskrun/sort/by_namespace.go Do not exist 100.0%

Comment thread pkg/taskrun/sort/by_namespace.go Outdated
Comment thread pkg/pipelinerun/sort/by_namespace.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You'll need to change check for if start time is nil:

if prs[j].Status.StartTime == nil {
	return false
}
if prs[i].Status.StartTime == nil {
	return true
}

@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch from 97628a8 to b9287d6 Compare March 19, 2020 20:58
@tekton-robot
Copy link
Copy Markdown
Contributor

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/pipelinerun/sort/by_namespace.go Do not exist 84.6%
pkg/taskrun/sort/by_namespace.go Do not exist 84.6%

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2020
Passing the --all-namespaces flag to the taskrun list
subcommands enables users to view taskruns in all namespaces
grouped by namespace and sorted by startime
Passing the --all-namespaces flag to the pipelinerun list
subcommands enables users to view pipelineruns in all namespaces
grouped by namespace and sorted by startime
@waveywaves waveywaves force-pushed the 785-list-all-namespaces branch from b9287d6 to ccbccc3 Compare March 20, 2020 01:19
@tekton-robot
Copy link
Copy Markdown
Contributor

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/pipelinerun/sort/by_namespace.go Do not exist 84.6%
pkg/taskrun/sort/by_namespace.go Do not exist 84.6%

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 Mar 20, 2020
@tekton-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [danielhelfand,piyush-garg]

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 merged commit bfd6390 into tektoncd:master Mar 20, 2020
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 lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support --all-namespaces flag for list commands

7 participants