Makes kubectl wait exit with status 1 and print an error message, if there is no resources matching selectors #66692

m1kola · 2018-07-26T22:50:51Z

What this PR does / why we need it:

It makes the kubectl wait command print an error message and exit with exit code 1, if there is no resource matching users's query. This can happen when user specifies selectors. Example:

kubectl wait deployment -l app=something-that-does-not-exist --for condition=available --timeout=5s

Which issue(s) this PR fixes:
Fixes #66456

Special notes for your reviewer:

This is my first contribution into the project (except one line change in docs) and don't have much experience with Go. I learned a lot while working on this (about resource finders and the Visitor interface and it's implementations), but it is very likely that I'm doing something wrong :)

I'm keen to continue contributing into the project (into the cli part for now), so I will really appreciate detailed feedback, if you have a chance to provide it (point me into a right direction and/or explain why it's not a good idea to do something in a certain way).

Thanks!

Release note:

kubectl: the wait command now prints an error message and exits with the code 1, if there is no resources matching selectors

neolit123

Added a couple of small comments.

neolit123 · 2018-07-27T22:34:19Z

pkg/kubectl/cmd/wait/wait.go

@@ -61,6 +62,9 @@ var (
 		kubectl wait --for=delete pod/busybox1 --timeout=60s`)
 )

+// ErrNoMatchingResources is returned when there is no resources matching a query.
+var ErrNoMatchingResources = errors.New("failed to find matching resources")


var -> const
If this is used only in this package you can lowercase

If this is used only in this package you can lowercase

Good idea, thanks. I've made it to be unexported.

var -> const

I think, the go compiler will complain about errors.New("something") not being a constant. I've stolen the approach from other packages where errors are defined as vars using errors.New(). Few examples:

kubernetes/pkg/client/conditions/conditions.go

Line 30 in d2721ec

var ErrPodCompleted = fmt.Errorf("pod ran to completion")

kubernetes/pkg/cloudprovider/providers/openstack/openstack.go

Lines 62 to 68 in d2721ec

var ErrNotFound = errors.New("failed to find object")

// ErrMultipleResults is used when we unexpectedly get back multiple results

var ErrMultipleResults = errors.New("multiple results where only one expected")

// ErrNoAddressFound is used when we cannot find an ip address for the host

var ErrNoAddressFound = errors.New("no address found for host")

neolit123 · 2018-07-27T22:37:17Z

pkg/kubectl/cmd/wait/wait.go

@@ -221,14 +227,24 @@ func (o *WaitOptions) RunWait() error {
 		}
 		return err
 	})
+
+	if err != nil {


You can remove this check and just "return err" at the end of the function.

You can also remove the empty lines. :)

You can remove this check and just "return err" at the end of the function.

Hm. I think, if I remove this condition it will break the existing behaviour. For example

kubectl wait deployment something-that-does-not-exist --for condition=available --timeout=5s

is currently printing an error from the API:

Error from server (NotFound): deployments.extensions "something-that-does-not-exist" not found

with the change you suggest to make it will printing "error: failed to find matching resources" in this case.

Another example (invalid command args):

kubectl wait deployment --for condition=available --timeout=5s

currently provides a helpful message: "error: resource(s) were provided, but no name, label selector, or --all flag specified", but if I remove the check it will also print "error: failed to find matching resources".

So, I think, we need to return errors from the visitors first.

neolit123 · 2018-07-27T22:42:22Z

Thanks for your PR @m1kola
Please make sure you run "./hack/verify-bazel.sh" and include the changes in the PR.

neolit123 · 2018-07-27T22:43:59Z

/sig cli

m1kola

@neolit123 thanks for the review.

I've made changes and left some comments. ./hack/verify-bazel.sh didn't generate any changes for this PR.

Could you, please, have another look? Thanks :)

m1kola · 2018-07-27T23:04:33Z

pkg/kubectl/cmd/wait/wait.go

@@ -221,14 +227,24 @@ func (o *WaitOptions) RunWait() error {
 		}
 		return err
 	})
+
+	if err != nil {


You can remove this check and just "return err" at the end of the function.

Hm. I think, if I remove this condition it will break the existing behaviour. For example

kubectl wait deployment something-that-does-not-exist --for condition=available --timeout=5s

is currently printing an error from the API:

Error from server (NotFound): deployments.extensions "something-that-does-not-exist" not found

with the change you suggest to make it will printing "error: failed to find matching resources" in this case.

Another example (invalid command args):

kubectl wait deployment --for condition=available --timeout=5s

currently provides a helpful message: "error: resource(s) were provided, but no name, label selector, or --all flag specified", but if I remove the check it will also print "error: failed to find matching resources".

So, I think, we need to return errors from the visitors first.

m1kola · 2018-07-27T23:22:53Z

pkg/kubectl/cmd/wait/wait.go

@@ -61,6 +62,9 @@ var (
 		kubectl wait --for=delete pod/busybox1 --timeout=60s`)
 )

+// ErrNoMatchingResources is returned when there is no resources matching a query.
+var ErrNoMatchingResources = errors.New("failed to find matching resources")


If this is used only in this package you can lowercase

Good idea, thanks. I've made it to be unexported.

var -> const

I think, the go compiler will complain about errors.New("something") not being a constant. I've stolen the approach from other packages where errors are defined as vars using errors.New(). Few examples:

kubernetes/pkg/client/conditions/conditions.go

Line 30 in d2721ec

var ErrPodCompleted = fmt.Errorf("pod ran to completion")

kubernetes/pkg/cloudprovider/providers/openstack/openstack.go

Lines 62 to 68 in d2721ec

var ErrNotFound = errors.New("failed to find object")

// ErrMultipleResults is used when we unexpectedly get back multiple results

var ErrMultipleResults = errors.New("multiple results where only one expected")

// ErrNoAddressFound is used when we cannot find an ip address for the host

var ErrNoAddressFound = errors.New("no address found for host")

neolit123 · 2018-07-28T09:10:47Z

Thanks for the update
/ok-to-test

neolit123 · 2018-07-29T23:25:21Z

@m1kola this looks good to me.
waiting on approval from @kubernetes/sig-cli-pr-reviews

soltysh

One small nit and you're good to go. Please squash your changes into a single commit as well.

soltysh · 2018-07-31T16:44:45Z

pkg/kubectl/cmd/wait/wait.go

@@ -61,6 +62,9 @@ var (
 		kubectl wait --for=delete pod/busybox1 --timeout=60s`)
 )

+// errNoMatchingResources is returned when there is no resources matching a query.
+var errNoMatchingResources = errors.New("failed to find matching resources")


no matching resources found

This makes `kubectl wait` print useful message when there is no resources matching a query. Also it will now exit with the exit status 1.

juanvallejo · 2018-07-31T17:30:39Z

/lgtm

m1kola · 2018-07-31T19:40:53Z

Thanks for the code review to everyone.

@soltysh I've updated the PR (error message changed and commits squashed). Tests are green ;)

mengqiy · 2018-08-06T15:43:15Z

/lgtm

k8s-ci-robot · 2018-08-06T15:43:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, m1kola, mengqiy

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:

~~pkg/kubectl/OWNERS~~ [mengqiy]

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

k8s-github-robot · 2018-08-06T16:13:53Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-08-06T17:31:57Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from eparis and mengqiy July 26, 2018 22:51

neolit123 reviewed Jul 27, 2018

View reviewed changes

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jul 27, 2018

m1kola commented Jul 27, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 28, 2018

soltysh approved these changes Jul 31, 2018

View reviewed changes

Return an error if there is no resources matching

d3445d7

This makes `kubectl wait` print useful message when there is no resources matching a query. Also it will now exit with the exit status 1.

k8s-ci-robot assigned juanvallejo Jul 31, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2018

k8s-ci-robot assigned mengqiy Aug 6, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2018

k8s-github-robot merged commit 5544187 into kubernetes:master Aug 6, 2018

m1kola deleted the 66456_waitcmd__error_for_selectors branch August 6, 2018 17:36

m1kola mentioned this pull request Oct 16, 2018

REQUEST: New membership for m1kola kubernetes/org#170

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes kubectl wait exit with status 1 and print an error message, if there is no resources matching selectors #66692

Makes kubectl wait exit with status 1 and print an error message, if there is no resources matching selectors #66692

m1kola commented Jul 26, 2018

neolit123 left a comment

neolit123 Jul 27, 2018

m1kola Jul 27, 2018

neolit123 Jul 27, 2018

neolit123 Jul 27, 2018

m1kola Jul 27, 2018 •

edited

Loading

neolit123 commented Jul 27, 2018

neolit123 commented Jul 27, 2018

m1kola left a comment

m1kola Jul 27, 2018 •

edited

Loading

m1kola Jul 27, 2018

neolit123 commented Jul 28, 2018

neolit123 commented Jul 29, 2018

soltysh left a comment

soltysh Jul 31, 2018

juanvallejo commented Jul 31, 2018

m1kola commented Jul 31, 2018

mengqiy commented Aug 6, 2018

k8s-ci-robot commented Aug 6, 2018

k8s-github-robot commented Aug 6, 2018

k8s-github-robot commented Aug 6, 2018

	var ErrNotFound = errors.New("failed to find object")

	// ErrMultipleResults is used when we unexpectedly get back multiple results
	var ErrMultipleResults = errors.New("multiple results where only one expected")

	// ErrNoAddressFound is used when we cannot find an ip address for the host
	var ErrNoAddressFound = errors.New("no address found for host")

Makes kubectl wait exit with status 1 and print an error message, if there is no resources matching selectors #66692

Makes kubectl wait exit with status 1 and print an error message, if there is no resources matching selectors #66692

Conversation

m1kola commented Jul 26, 2018

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Jul 27, 2018

Choose a reason for hiding this comment

m1kola Jul 27, 2018

Choose a reason for hiding this comment

neolit123 Jul 27, 2018

Choose a reason for hiding this comment

neolit123 Jul 27, 2018

Choose a reason for hiding this comment

m1kola Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

neolit123 commented Jul 27, 2018

neolit123 commented Jul 27, 2018

m1kola left a comment

Choose a reason for hiding this comment

m1kola Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

m1kola Jul 27, 2018

Choose a reason for hiding this comment

neolit123 commented Jul 28, 2018

neolit123 commented Jul 29, 2018

soltysh left a comment

Choose a reason for hiding this comment

soltysh Jul 31, 2018

Choose a reason for hiding this comment

juanvallejo commented Jul 31, 2018

m1kola commented Jul 31, 2018

mengqiy commented Aug 6, 2018

k8s-ci-robot commented Aug 6, 2018

k8s-github-robot commented Aug 6, 2018

k8s-github-robot commented Aug 6, 2018

m1kola Jul 27, 2018 •

edited

Loading

m1kola Jul 27, 2018 •

edited

Loading