UPSTREAM: 32722: warn on empty oc get output #10915

juanvallejo · 2016-09-14T20:37:24Z

Fixes: #10285
Related Trello Card:
https://trello.com/c/04lpfTQR/451-8-cli-improved-flows-and-ux-in-the-cli-evg-ux-p3

The current default behavior of oc get is to return an empty
output when there are no resources to display. This patch improves
usability by returning a warning through stderr in the case of an empty
list.

Before

$ oc get roles
empty output

After

oc get roles

There are no resources to display.

cc @openshift/cli-review

juanvallejo · 2016-09-14T21:18:41Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/get.go

+
+		// output empty list warning when no resources to display
+		if len(infos) == 0 {
+			fmt.Fprintf(os.Stderr, "%s\n", "There are no resources to display.")


@fabianofranz do you think it would make more sense to output the current namespace / context instead of this? (There are two checkboxes on Trello that ask for more awareness of the current namespace / context and thought this might be a good place to at least partially address those: https://trello.com/c/04lpfTQR/451-8-cli-improved-flows-and-ux-in-the-cli-evg-ux-p3)

EDIT: the new text could read something similar to: "There are no resources to display. You are currently logged in as ... on project ..."

I don't think we need to print the user, but the namespace seems like a good idea. Even because "user" and "logged in" are not concepts as clear in Kube as they are in Origin, it's more like the session token. Also, remember that "project" is not a concept in Kube, we need to refer to it as "namespaces". So maybe something like

No resources to display in namespace "mynamespace". No pod(s) to display in namespace "mynamespace".

Resource name might be harder given all, plural/singular etc.

Remember that when running get with flags that filter the result, it may be misguiding to say "no resources" , "no pods", etc; it can give the wrong impression that there are no resources when actually just there aren't resources that match the provided criteria. I'd like to use some wording that makes it clear. Maybe

$ oc get all --selector="app=awesome" No resources found in namespace "mynamespace" that match your criteria.

Ping @liggitt my Personal Assistant For Grammar Questions™.

Do we refer to them as 'filters' in the flag descriptions? If so I would probably say this instead.

No resources found in namespace "mynamespace" that match your filters.

If the only flag that filters is selector then i'd say

No resources found in namespace "mynamespace" that matches the selector.

But I would only be that specific if we know they used the flag. If this output is in a generic place then be generic and go with criteria or filters.

fabianofranz · 2016-09-14T21:47:05Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/get.go

+
+	// output empty list warning when no resources to display
+	if len(objs) == 0 {
+		fmt.Fprintf(os.Stderr, "%s\n", "There are no resources to display.")


BTW we need to make sure Stdout, Stderr, etc are passed to commands by the root command and that we use what was passed in every command. The entire CLI structure is pretty screwed in that sense. I'm addressing it in one of my PRs.

Okay, I'll go ahead and pass outErr through the root cmd. Thanks!

juanvallejo · 2016-09-19T14:27:06Z

Linking @smarterclayton's comment from upstream PR: kubernetes/kubernetes#32722 (comment)

fabianofranz · 2016-09-21T15:54:13Z

LGTM, wait for upstream.

juanvallejo · 2016-10-13T13:20:16Z

@fabianofranz upstream merged :) kubernetes/kubernetes#32722 PTAL

fabianofranz · 2016-10-13T18:36:43Z

[merge], nice usability improvement.

juanvallejo · 2016-10-17T18:14:40Z

@fabianofranz test/cmd/sdn.sh is updated now to expect text on "empty" oc get ... output

fabianofranz · 2016-10-17T18:17:13Z

@juanvallejo thanks, [merge].

openshift-bot · 2016-10-17T18:28:04Z

[Test]ing while waiting on the merge queue

juanvallejo · 2016-10-20T13:49:33Z

conformance test flaked on #9548 re[test]

fabianofranz · 2016-10-20T13:50:15Z

flaked on #9548 re[merge]

juanvallejo · 2016-10-20T21:17:14Z

@fabianofranz integration test flaked on #9959, conformance had a devicemapper flake #9548

fabianofranz · 2016-10-21T13:53:03Z

flaked on #9959 and #9548 re[merge]

fabianofranz · 2016-10-21T20:02:06Z

flaked on #11442 re[merge]

juanvallejo · 2016-10-24T13:30:58Z

conformance devicemapper flake #9548

fabianofranz · 2016-10-24T13:32:27Z

flaked on #9548 re[merge]

fabianofranz · 2016-10-24T18:58:49Z

#8571 re[test] re[merge]

juanvallejo · 2016-10-25T14:01:13Z

conformance devicemapper flake #9548

fabianofranz · 2016-10-25T15:29:59Z

flake #9548 re[merge]

fabianofranz · 2016-10-25T18:54:39Z

#11560 re[test] re[merge]

fabianofranz · 2016-10-27T18:57:42Z

conformance flaked on #10663 re[test] re[merge]

juanvallejo · 2016-10-28T14:28:54Z

conformance test flaked on #9548

fabianofranz · 2016-10-28T14:39:40Z

#9548 re[test] re[merge]

openshift-bot · 2016-10-28T16:53:23Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10946/) (Image: devenv-rhel7_5291)

fabianofranz · 2016-10-28T18:04:08Z

@juanvallejo I think the latest conformance error might be legitimate. The following bits are expecting an empty output (which they'll never get):

https://github.com/openshift/origin/blob/master/test/extended/builds/remove_buildconfig.go#L48-L53

juanvallejo · 2016-10-28T19:51:41Z

@fabianofranz Thanks! I will take a look

Fixes: openshift#10285 Related Trello Card: https://trello.com/c/04lpfTQR/451-8-cli-improved-flows-and-ux-in-the-cli-evg-ux-p3 The current default behavior of `oc get` is to return an empty output when there are no resources to display. This patch improves usability by returning a warning through stderr in the case of an empty list.

juanvallejo · 2016-11-01T17:41:00Z

@fabianofranz The test keeps failing here but I am not sure if this is due to a change I made in this PR, or if it's just a flake

juanvallejo · 2016-11-01T18:51:15Z

conformance flaked on #11661 re[test]

openshift-bot · 2016-11-01T18:53:28Z

Evaluated for origin test up to 26083d5

openshift-bot · 2016-11-01T20:23:18Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10946/) (Base Commit: f55922e)

fabianofranz · 2016-11-01T20:33:04Z

[merge]

openshift-bot · 2016-11-01T20:34:06Z

Evaluated for origin merge up to 26083d5

juanvallejo force-pushed the jvallejo_return-err-on-oc-get-empty-list branch from eee2f49 to fbb8a1f Compare September 14, 2016 21:03

juanvallejo commented Sep 14, 2016

View reviewed changes

fabianofranz reviewed Sep 14, 2016

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016

juanvallejo force-pushed the jvallejo_return-err-on-oc-get-empty-list branch from 4e00c30 to f19f4ac Compare September 16, 2016 21:47

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2016

juanvallejo force-pushed the jvallejo_return-err-on-oc-get-empty-list branch from 2449162 to ec5fdc4 Compare September 27, 2016 17:50

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2016

juanvallejo force-pushed the jvallejo_return-err-on-oc-get-empty-list branch from ec5fdc4 to fe64e11 Compare October 17, 2016 18:02

juanvallejo force-pushed the jvallejo_return-err-on-oc-get-empty-list branch from fe64e11 to 6fcec58 Compare October 25, 2016 20:44

juanvallejo force-pushed the jvallejo_return-err-on-oc-get-empty-list branch 2 times, most recently from 55a9ce0 to 8cc4387 Compare October 31, 2016 14:02

juanvallejo added 3 commits November 1, 2016 09:38

update e2e test to work with non-emtpy output

d918a8d

update extended tests to work with non-empty output

26083d5

juanvallejo force-pushed the jvallejo_return-err-on-oc-get-empty-list branch from 8cc4387 to 26083d5 Compare November 1, 2016 13:38

openshift-bot merged commit 770ba8b into openshift:master Nov 1, 2016

juanvallejo deleted the jvallejo_return-err-on-oc-get-empty-list branch November 1, 2016 22:44

UPSTREAM: 32722: warn on empty oc get output #10915

UPSTREAM: 32722: warn on empty oc get output #10915

Conversation

juanvallejo commented Sep 14, 2016 • edited Loading

Before

After

juanvallejo Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

fabianofranz Sep 14, 2016

Choose a reason for hiding this comment

jwforres Sep 16, 2016

Choose a reason for hiding this comment

fabianofranz Sep 14, 2016

Choose a reason for hiding this comment

juanvallejo Sep 15, 2016

Choose a reason for hiding this comment

juanvallejo commented Sep 19, 2016

fabianofranz commented Sep 21, 2016

juanvallejo commented Oct 13, 2016

fabianofranz commented Oct 13, 2016

juanvallejo commented Oct 17, 2016

fabianofranz commented Oct 17, 2016

openshift-bot commented Oct 17, 2016

juanvallejo commented Oct 20, 2016

fabianofranz commented Oct 20, 2016

juanvallejo commented Oct 20, 2016

fabianofranz commented Oct 21, 2016

fabianofranz commented Oct 21, 2016

juanvallejo commented Oct 24, 2016

fabianofranz commented Oct 24, 2016

fabianofranz commented Oct 24, 2016

juanvallejo commented Oct 25, 2016

fabianofranz commented Oct 25, 2016

fabianofranz commented Oct 25, 2016

fabianofranz commented Oct 27, 2016

juanvallejo commented Oct 28, 2016

fabianofranz commented Oct 28, 2016

openshift-bot commented Oct 28, 2016 • edited Loading

fabianofranz commented Oct 28, 2016

juanvallejo commented Oct 28, 2016

juanvallejo commented Nov 1, 2016

juanvallejo commented Nov 1, 2016

openshift-bot commented Nov 1, 2016

openshift-bot commented Nov 1, 2016

fabianofranz commented Nov 1, 2016

openshift-bot commented Nov 1, 2016

juanvallejo commented Sep 14, 2016 •

edited

Loading

juanvallejo Sep 14, 2016 •

edited

Loading

openshift-bot commented Oct 28, 2016 •

edited

Loading