Add Namespace Check to Task Delete Command #350

danielhelfand · 2019-10-08T20:26:54Z

This pull request follows up #349 and also helps to address #311. The goal is to check whether a namespace exists to alert the user if the namespace provided is invalid.

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)
Regenerate the manpages and docs with make docs and make man if needed.
Run the code checkers with make check
Commit messages follow commit message best practices

Release Notes

Adds error message to tkn task delete when a namespace does not exist

tekton-robot · 2019-10-08T20:27:09Z

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

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.

pkg/cmd/task/delete.go

sthaha

/ok-to-test

tekton-robot · 2019-10-09T00:59:11Z

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/task/delete.go	90.9%	91.5%	0.6

tekton-robot · 2019-10-09T01:42:16Z

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/task/delete.go	90.9%	91.5%	0.6

sthaha · 2019-10-09T03:45:40Z

pkg/cmd/task/delete.go

-	if err != nil {
-		return fmt.Errorf("failed to create tekton client")
-	}
+func deleteTask(opts *deleteOptions, s *cli.Stream, p cli.Params, cs *cli.Clients, tName string) error {


isn't cs already in p cli.Params ?

Yes it is. I'll point on in a review comment why I went with this approach, but the gist of it was I was trying to avoid checking for the "failed to create tekton client" error twice when using p.Clients().

IMHO, ... it is okay for internal (non exported methods) to make assumptions about arguments it receives ; i.e. the caller should have ensured the arguments are valid before passing it to callee so that certain step need to be performed only once ... (premature optimisation 🤷‍♂️ ; may be ...)

sthaha · 2019-10-09T03:52:29Z

pkg/cmd/task/delete.go

+				return fmt.Errorf("failed to create tekton client")
+			}
+
+			// Check if namespace exists. Return error if namespace specified with -n doesn't exist or if user doesn't have permissions to view.


it may be better to move these validations to a package validate so that the call appears

if err := validate.NamespaceExists(kubeClient, ns); err != nil{ return err }

WDYT?

I have moved the validation code under pkg/helper/validate. Let me know what you think.

sthaha

How about a validate package to keep all common validations. This may be also be useful in tests as well.

tekton-robot · 2019-10-09T13:59:57Z

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/task/delete.go	90.9%	89.8%	-1.1
pkg/cmd/task/list.go	67.4%	66.7%	-0.8
pkg/helper/validate/validate.go	Do not exist	100.0%

danielhelfand · 2019-10-09T14:10:13Z

pkg/cmd/task/list.go

@@ -70,16 +80,9 @@ func listCommand(p cli.Params) *cobra.Command {
 }

 func printTaskDetails(s *cli.Stream, p cli.Params) error {
-
 	cs, err := p.Clients()
 	if err != nil {


If there's a way to avoid the second error check here, would be nice. Was originally passing the clients as a param to printTaskDetails, but I'm not opposed to two checks.

Agree, I was also thinking of

validate.All( validate.Clients(p), validate.NamespaceExists(p))

where validate.KubeClient(p) returns a validator function like: func validator() error and validate.All just checks none of the functions return an error`

That way all commands would just have to invoke just

if err := validate.All( validate.Clients(p), validate.Foobar(x, y, z)); err != nil { return err }

WDYT?

BTW, I am not suggesting this change in this PR :)

Sure, I think that's really clever. I'll keep this in mind a try to refactor it.

danielhelfand · 2019-10-09T14:10:55Z

pkg/helper/validate/validate.go

@@ -0,0 +1,32 @@
+// Copyright © 2019 The Tekton Authors.


Idea here would be to use this class for common validation methods across the CLI.

sthaha

/lgtm

tekton-robot · 2019-10-10T01:57:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sthaha

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~~ [sthaha]

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

tekton-robot requested review from hrishin and sthaha October 8, 2019 20:26

googlebot added the cla: yes label Oct 8, 2019

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 Oct 8, 2019

sthaha reviewed Oct 9, 2019

View reviewed changes

pkg/cmd/task/delete.go Outdated Show resolved Hide resolved

sthaha reviewed Oct 9, 2019

View reviewed changes

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 Oct 9, 2019

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 9, 2019

sthaha reviewed Oct 9, 2019

View reviewed changes

sthaha suggested changes Oct 9, 2019

View reviewed changes

add namespace check to task delete command

291532c

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 9, 2019

danielhelfand commented Oct 9, 2019

View reviewed changes

sthaha reviewed Oct 10, 2019

View reviewed changes

tekton-robot assigned sthaha Oct 10, 2019

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

sthaha approved these changes Oct 10, 2019

View reviewed changes

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

tekton-robot merged commit 74f5638 into tektoncd:master Oct 10, 2019

danielhelfand mentioned this pull request Oct 11, 2019

Add Namespace Check for Task Describe Command #355

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Namespace Check to Task Delete Command #350

Add Namespace Check to Task Delete Command #350

danielhelfand commented Oct 8, 2019

tekton-robot commented Oct 8, 2019

sthaha left a comment

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

sthaha Oct 9, 2019

danielhelfand Oct 9, 2019

sthaha Oct 10, 2019

sthaha Oct 9, 2019

danielhelfand Oct 9, 2019

sthaha left a comment

tekton-robot commented Oct 9, 2019

danielhelfand Oct 9, 2019

sthaha Oct 10, 2019

danielhelfand Oct 10, 2019

danielhelfand Oct 9, 2019

sthaha left a comment

tekton-robot commented Oct 10, 2019

Add Namespace Check to Task Delete Command #350

Add Namespace Check to Task Delete Command #350

Conversation

danielhelfand commented Oct 8, 2019

Submitter Checklist

Release Notes

tekton-robot commented Oct 8, 2019

sthaha left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 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

Choose a reason for hiding this comment

sthaha left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 9, 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

tekton-robot commented Oct 10, 2019