Add functionality to list clustertasks #161

onyiny-ang · 2019-07-11T22:28:51Z

Changes

Addresses #118: Adding functionality to list ClusterTasks

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)
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Added support for listing clustertasks with `tkn clustertask list`

googlebot · 2019-07-11T22:28:53Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

vdemeester · 2019-07-12T07:03:00Z

/ok-to-test

vdemeester

@onyiny-ang Looking good 👼 👍

tekton-robot · 2019-07-12T07:06:18Z

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/cmd/clustertask/clustertask.go	Do not exist	100.0%
pkg/cmd/clustertask/list.go	Do not exist	67.4%

hrishin · 2019-07-12T09:18:16Z

/test pull-tekton-cli-build-tests

- Lists NAME and AGE of clustertasks - Follows existing tkn command structure - Closes tektoncd#118 Just adjust the vendor modules

googlebot · 2019-07-15T16:08:01Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

tekton-robot · 2019-07-15T16:10:37Z

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/cmd/clustertask/clustertask.go	Do not exist	100.0%
pkg/cmd/clustertask/list.go	Do not exist	67.4%

sthaha · 2019-07-16T04:11:56Z

pkg/cmd/clustertask/clustertask.go

+func Command(p cli.Params) *cobra.Command {
+	cmd := &cobra.Command{
+		Use:     "clustertask",
+		Aliases: []string{"c", "clustertasks"},


How about ct instead of c

sthaha · 2019-07-16T04:13:19Z

pkg/cmd/clustertask/list.go

+	}
+
+	clustertasks, err := listAllClustertasks(cs.Tekton)
+


nit: may want to remove the blank line

sthaha · 2019-07-16T04:15:13Z

pkg/cmd/clustertask/list.go

+		return nil, err
+	}
+
+	if len(clustertasks.Items) == 0 {


isn't this if superfluous? i.e. wouldn't the output be the same if we deleted it?

Removed from this and task list.go in follow up commit

sthaha

Thanks for the patch ❤️

looks good overall but minor comments to address

vdemeester · 2019-07-16T07:27:10Z

pkg/cmd/clustertask/list.go

+		Use:     "list",
+		Aliases: []string{"ls"},
+		Short:   "Lists clustertasks in a namespace",
+		Long:    ``,


We don't need to add the Long field if it's empty 😝 (we may need to fix that on other commands)

Fixed and also deleted the "Long" flag from pipelines and task list.go `

- Removes "Long" command flags that are left blank - Removes superflous "if" statement

tekton-robot · 2019-07-16T13:52:18Z

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/cmd/clustertask/clustertask.go	Do not exist	100.0%
pkg/cmd/clustertask/list.go	Do not exist	65.9%
pkg/cmd/task/list.go	67.4%	65.9%	-1.5

vdemeester

/lgtm

tekton-robot · 2019-07-16T13:53:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: onyiny-ang, 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:

~~OWNERS~~ [vdemeester]

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

sthaha · 2019-07-16T14:03:44Z

pkg/cmd/clustertask/list.go

+	return clustertasks, nil
+}
+
+func listClustertaskDetails(cs versioned.Interface) (*v1alpha1.ClusterTaskList, error) {


Could you please help me understand why we need this func ? isn't this the same as listAllClusterTasks?

Addressed in #165

tekton-robot requested review from sthaha and vdemeester July 11, 2019 22:28

googlebot added the cla: no label Jul 11, 2019

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 Jul 11, 2019

tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 12, 2019

vdemeester reviewed Jul 12, 2019

View reviewed changes

Add functionality to list clustertasks

46282ac

- Lists NAME and AGE of clustertasks - Follows existing tkn command structure - Closes tektoncd#118 Just adjust the vendor modules

onyiny-ang force-pushed the clustertasks branch from f5f7469 to 46282ac Compare July 15, 2019 16:07

googlebot added cla: yes and removed cla: no labels Jul 15, 2019

sthaha reviewed Jul 16, 2019

View reviewed changes

sthaha suggested changes Jul 16, 2019

View reviewed changes

vdemeester reviewed Jul 16, 2019

View reviewed changes

onyiny-ang added 2 commits July 16, 2019 09:38

Address code review--will squash on approval

4c4a2ed

Clean up superflous code from pipeline and task resources

a23797c

- Removes "Long" command flags that are left blank - Removes superflous "if" statement

tekton-robot assigned vdemeester Jul 16, 2019

vdemeester approved these changes Jul 16, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2019

tekton-robot merged commit e0dea38 into tektoncd:master Jul 16, 2019

sthaha reviewed Jul 16, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality to list clustertasks #161

Add functionality to list clustertasks #161

onyiny-ang commented Jul 11, 2019

googlebot commented Jul 11, 2019

vdemeester commented Jul 12, 2019

vdemeester left a comment

tekton-robot commented Jul 12, 2019

hrishin commented Jul 12, 2019

googlebot commented Jul 15, 2019

tekton-robot commented Jul 15, 2019

sthaha Jul 16, 2019

sthaha Jul 16, 2019

sthaha Jul 16, 2019

onyiny-ang Jul 16, 2019

sthaha left a comment

vdemeester Jul 16, 2019

onyiny-ang Jul 16, 2019

tekton-robot commented Jul 16, 2019

vdemeester left a comment

tekton-robot commented Jul 16, 2019

sthaha Jul 16, 2019

onyiny-ang Jul 16, 2019

Add functionality to list clustertasks #161

Add functionality to list clustertasks #161

Conversation

onyiny-ang commented Jul 11, 2019

Changes

Submitter Checklist

Release Notes

googlebot commented Jul 11, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

vdemeester commented Jul 12, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 12, 2019

hrishin commented Jul 12, 2019

googlebot commented Jul 15, 2019

tekton-robot commented Jul 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jul 16, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment