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

Add resource list command #128

Merged
merged 1 commit into from
Jul 3, 2019
Merged

Conversation

piyush-garg
Copy link
Contributor

@piyush-garg piyush-garg commented Jun 26, 2019

Changes

This will add resource list command to
list all the pipelineresource available in the default
or provided namespace

It will show NAME and TYPE fields

Add docs regarding the new command

Add e2e test

This will fix #108

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

Add `tkn resource list` command

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 26, 2019
@piyush-garg piyush-garg changed the title Iss 108 Add pipelineresource list command Jun 26, 2019
@hrishin
Copy link
Member

hrishin commented Jun 26, 2019

/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 Jun 26, 2019
pkg/cmd/pipelineresource/list.go Outdated Show resolved Hide resolved

options := v1.ListOptions{}
prec := cs.Tekton.TektonV1alpha1().PipelineResources(p.Namespace())
pres, err := prec.List(options)
Copy link
Member

@sthaha sthaha Jun 26, 2019

Choose a reason for hiding this comment

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

How about we group the objects by type?
I also wonder if we can ensure the order remains the same between subsequent invocations; perhaps sorted by time?.

Copy link
Member

Choose a reason for hiding this comment

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

@sthaha you mean type: ImageResource for a instance (resource.spec.type)`?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. We can work this out in a follow-up :

  • Be able to group resources
  • Be able to filter based on resource types

pkg/cmd/root_test.go Outdated Show resolved Hide resolved
@piyush-garg
Copy link
Contributor Author

/test pull-tekton-cli-go-coverage

@hrishin
Copy link
Member

hrishin commented Jun 26, 2019

/test pull-tekton-cli-go-coverage

@piyush-garg
Copy link
Contributor Author

@hrishin I have addressed your review comment.

pkg/cmd/pipelineresource/list.go Outdated Show resolved Hide resolved
pkg/cmd/pipelineresource/list.go Outdated Show resolved Hide resolved
Copy link
Member

@hrishin hrishin left a comment

Choose a reason for hiding this comment

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

👍

We need to add a few more test cases.

@piyush-garg
Copy link
Contributor Author

@hrishin Addressed

Copy link
Member

@hrishin hrishin left a comment

Choose a reason for hiding this comment

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

few more comments (please)

docs/cmd/tkn_resource_list.md Outdated Show resolved Hide resolved
docs/cmd/tkn_resource_list.md Outdated Show resolved Hide resolved
pkg/cmd/pipelineresource/list.go Outdated Show resolved Hide resolved
pkg/cmd/pipelineresource/list.go Outdated Show resolved Hide resolved
pkg/cmd/pipelineresource/list_test.go Show resolved Hide resolved
pkg/cmd/pipelineresource/list_test.go Show resolved Hide resolved
@piyush-garg piyush-garg changed the title Add pipelineresource list command Add resource list command Jun 28, 2019
Copy link
Member

@hrishin hrishin left a comment

Choose a reason for hiding this comment

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

just last thing @piyush-garg ? then looks good

pkg/cmd/pipelineresource/list.go Show resolved Hide resolved
@hrishin
Copy link
Member

hrishin commented Jun 28, 2019

/LGTM

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2019
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2019
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that 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 here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 2, 2019

CLA Check
Thank you. Unfortunately, your account is not authorized under a signed CLA. Please click here to proceed.

  • Piyush Garg (0faecf27658cb820bc7188bbf72a8cd68799f525)

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

This will add resource list command to
list all the pipelineresource available in the default
or provided namespace

It will show NAME and TYPE fields

Add docs regarding the new command

Add e2e test

This will fix tektoncd#108
@hrishin
Copy link
Member

hrishin commented Jul 3, 2019

@sthaha @vdemeester one more review round?

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.

/lgtm
/meow boxes

We can enhance in follow-ups

### Options inherited from parent commands

```
--azure-container-registry-config string Path to the file containing Azure container registry configuration information.
Copy link
Member

Choose a reason for hiding this comment

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

nit: we need to prepare a follow-up to remove this flag everywhere in the docs 😹

Copy link
Member

Choose a reason for hiding this comment

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

👍


options := v1.ListOptions{}
prec := cs.Tekton.TektonV1alpha1().PipelineResources(p.Namespace())
pres, err := prec.List(options)
Copy link
Member

Choose a reason for hiding this comment

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

Good point. We can work this out in a follow-up :

  • Be able to group resources
  • Be able to filter based on resource types

Use: "tkn",
Short: "CLI for tekton pipelines",
Long: ``,
Use: "tkn",
Copy link
Member

Choose a reason for hiding this comment

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

This changes is weird (aka I don't see why goimports or gofmt would do that)

@tekton-robot
Copy link
Contributor

@vdemeester: Bad category. Please see https://api.thecatapi.com/api/categories/list

In response to this:

/lgtm
/meow boxes

We can enhance in follow-ups

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 the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019
@tekton-robot
Copy link
Contributor

[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

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 Jul 3, 2019
@tekton-robot tekton-robot merged commit e05047e into tektoncd:master Jul 3, 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 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
Development

Successfully merging this pull request may close these issues.

List PipelineResources
6 participants