do not filter kubectl get pods if -o json or yaml #39042

juanvallejo · 2016-12-20T20:37:21Z

This patch sets the value of --show-all to true if the output format
specified is 'json' or 'yaml'.

Release note:

release-note-none

@smarterclayton

k8s-ci-robot · 2016-12-20T20:37:22Z

Hi @juanvallejo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

k8s-reviewable · 2016-12-20T20:37:29Z

This change is

pwittrock · 2017-01-03T16:32:18Z

@k8s-bot ok to test

smarterclayton · 2017-01-03T17:14:15Z

pkg/kubectl/cmd/get.go

@@ -187,7 +187,8 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 	if err != nil {
 		return err
 	}
-	if len(options.Filenames) > 0 || argsHasNames {
+	output := cmdutil.GetFlagString(cmd, "output")
+	if len(options.Filenames) > 0 || argsHasNames || output == "json" || output == "yaml" {


Probably would be better to default show-all to true, rather than force it to true.

I.e. kubectl get pods -o yaml --show-all=false should filter pods.

juanvallejo · 2017-01-09T16:53:19Z

@smarterclayton thanks for the feedback, updated the default value (and flag description) of show-all to true e1ec3d8

k8s-ci-robot · 2017-01-09T17:19:01Z

Jenkins unit/integration failed for commit e1ec3d80991937ce92a0eddb6844dbe487693153. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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. I understand the commands that are listed here.

pwittrock · 2017-01-10T16:39:32Z

@juanvallejo I think we only want to change the default if -o json or yaml is specified. I think you should be able to check if a flag value has been specified or not (different than checking the value). You can then keep your changes as before, but guard setting "true" on if it has not been explicitly set already.

This patch sets the value of --show-all to true if the output format specified is 'json' or 'yaml'.

juanvallejo · 2017-01-10T19:44:47Z

@pwittrock Thanks for the feedback, updated original commit to check if show-all flag value was set by the user before forcing its value to true

k8s-ci-robot · 2017-01-10T21:13:47Z

Jenkins kops AWS e2e failed for commit cade00e. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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. I understand the commands that are listed here.

pwittrock · 2017-01-10T23:29:38Z

@k8s-bot kops aws e2e test this

pwittrock · 2017-01-10T23:29:58Z

@k8s-bot unit test this

juanvallejo · 2017-01-12T19:27:50Z

@k8s-bot bazel test this

fabianofranz · 2017-01-16T15:17:16Z

pkg/kubectl/cmd/get.go

-	if len(options.Filenames) > 0 || argsHasNames {
-		cmd.Flag("show-all").Value.Set("true")
+	output := cmdutil.GetFlagString(cmd, "output")
+	if len(options.Filenames) > 0 || argsHasNames || output == "json" || output == "yaml" {


What happens if --output is go-template* or jsonpath*? I believe it should apply to all too (no filtering). Also, I'd suggest to move the output check to a helper function like OutputsRawFormat(output string) bool or something similar.

juanvallejo · 2017-01-16T19:00:17Z

@fabianofranz thanks for the feedback, added a OutputsRawFormat helper which returns true if any machine-parsable output was specified, and also defaulted show-all to true if the all alias was passed

k8s-ci-robot · 2017-01-16T20:00:17Z

Jenkins GCI GCE e2e failed for commit 34c30ca. Full PR test history. cc @juanvallejo

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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. I understand the commands that are listed here.

juanvallejo · 2017-01-16T20:23:01Z

@fabianofranz GCI GCE flaked on #39973

fabianofranz · 2017-01-17T14:51:34Z

@k8s-bot gci gce e2e test this

pwittrock · 2017-01-17T21:47:34Z

pkg/kubectl/cmd/get.go

-	// always show resources when getting by name or filename
+	// determine if args contains "all"
+	for _, a := range args {
+		if a == "all" {


This will match situations where resources are named 'all'

kubectl get pods all

I'd either address this in a follow up for a quicker approval and submission, or fix it by matching the parsing logic for the 'all' keyword

@pwittrock thanks for the feedback. I removed this and just use the boolean value of resource.MultipleTypesRequested.

cc @fabianofranz this would mean that passing the all alias or requesting mixed resource types would cause the value of show-all to default to true. wdyt?

ug, it looks like that code has the same issue. I am not sure if there is a great solution:

kubernetes/pkg/kubectl/resource/builder.go

Line 808 in 6e5b455

func MultipleTypesRequested(args []string) bool {

Sorry if this was covered in another comment. What is the motivation for showing all when printing all resource types? This is unrelated to the output type.

What is the motivation for showing all when printing all resource types? This is unrelated to the output type.

Although unrelated to the output type, I was thinking it would be a better user experience overall if all implied that even resources that are hidden by default would also be displayed.

it looks like that code has the same issue. I am not sure if there is a great solution:

kubernetes/pkg/kubectl/resource/builder.go

Line 808 in 6e5b455

func MultipleTypesRequested(args []string) bool {

Would it be a valid assumption that if all comes before any other "kind", it can be implied that the user did mean "all resources", rather than pod/all? If this is the case, could we only check to see if args[0] == "all", or would that be too fragile?

Although unrelated to the output type, I was thinking it would be a better user experience overall if all implied that even resources that are hidden by default would also be displayed.

This might be a lot of things, since this is showing both all resource types and hidden resources.

Would it be a valid assumption that if all comes before any other "kind", it can be implied that the user did mean "all resources", rather than pod/all? If this is the case, could we only check to see if args[0] == "all", or would that be too fragile?

Maybe we should check there is only 1 argument, and it is all?

if len(args) == 1 && args[0] == "all"

Maybe we should check there is only 1 argument, and it is all?

Sounds good to me, I can go ahead and open a PR to patch this in the MultipleTypesRequested helper

This might be a lot of things, since this is showing both all resource types and hidden resources.

This makes sense, I'll remove the check for all from the logic for the default value of show-all

Maybe we should check there is only 1 argument, and it is all?

Remember you may be interesting in getting all + something that is not in all, in which case you would have for example kubectl get events,all. Seems to break the proposed logic.

@fabianofranz

Remember you may be interesting in getting all + something that is not in all, in which case you would have for example kubectl get events,all. Seems to break the proposed logic.

This is a good point, I am updating the logic for MultipleTypesRequested in this PR, and while specifying something like $ kubectl get pods,all does not fulfill the base case check of if len(args) == 1 && args[0] == "all", the rest of the logic for that function does correctly detect multiple types in cases like this one when it splits the string by , and finds more than one "resource" was specified. Although this would no longer be work for this PR (if we had just decided to check for "all" using len(args) == 1 && args[0] == "all" rather than calling the entire MultipleTypesRequested helper)

pwittrock · 2017-01-18T16:54:57Z

SGTM. Let me know when it is updated. (removing multiple resource types piece)

juanvallejo · 2017-01-18T18:27:13Z

@pwittrock

SGTM. Let me know when it is updated. (removing multiple resource types piece)

Done!

pwittrock · 2017-01-18T20:30:13Z

/lgtm

k8s-github-robot · 2017-01-18T21:37:13Z

Automatic merge from submit-queue (batch tested with PRs 36467, 36528, 39568, 40094, 39042)

smarterclayton · 2017-03-01T23:19:24Z

After looking at this and staring at it for a bit while fixing other bugs, I think I was wrong to want this. It makes behavior of get inconsistent and is surprising. Users should learn to use -a.

I'm going to open a PR to revert this behavior for 1.6 unless anyone objects.

smarterclayton · 2017-03-01T23:20:01Z

Since this has CLI API implications, I'd prefer to make a decision for 1.6 so we don't have to live with it.

smarterclayton · 2017-03-01T23:21:52Z

Instead, we should simply ensure we don't show the message for filtering for generic output.

Automatic merge from submit-queue Output of `kubectl get` is inconsistent for pods Builds on top of fixes from #42283, only the last two commits are new. Reverts behavior of #39042 which was inconsistent and confusing. Fixes #15853

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 20, 2016

k8s-github-robot assigned pwittrock Dec 20, 2016

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 20, 2016

smarterclayton reviewed Jan 3, 2017

View reviewed changes

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 9, 2017

juanvallejo force-pushed the jvallejo/dont-filter-pods-if-json-or-yaml branch from e1ec3d8 to e3ca1e7 Compare January 9, 2017 18:27

do not filter kubectl get pods if -o json or yaml

cade00e

This patch sets the value of --show-all to true if the output format specified is 'json' or 'yaml'.

juanvallejo force-pushed the jvallejo/dont-filter-pods-if-json-or-yaml branch from e3ca1e7 to cade00e Compare January 10, 2017 19:42

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 10, 2017

fabianofranz reviewed Jan 16, 2017

View reviewed changes

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 16, 2017

pwittrock reviewed Jan 17, 2017

View reviewed changes

add OutputsRawFormat helper; default show-all on multiple resource kinds

430283b

juanvallejo force-pushed the jvallejo/dont-filter-pods-if-json-or-yaml branch from 34c30ca to 430283b Compare January 17, 2017 22:07

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 17, 2017

remove check for multiple resource types

713df3d

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2017

k8s-github-robot merged commit 4520819 into kubernetes:master Jan 18, 2017

juanvallejo deleted the jvallejo/dont-filter-pods-if-json-or-yaml branch January 18, 2017 22:04

juanvallejo mentioned this pull request Jan 20, 2017

update check for "all" resources #40046

Merged

enisoc added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 27, 2017

smarterclayton mentioned this pull request Mar 1, 2017

Output of kubectl get is inconsistent for pods #42369

Merged

do not filter kubectl get pods if -o json or yaml #39042

do not filter kubectl get pods if -o json or yaml #39042

Conversation

juanvallejo commented Dec 20, 2016

k8s-ci-robot commented Dec 20, 2016

k8s-reviewable commented Dec 20, 2016

pwittrock commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Jan 9, 2017

k8s-ci-robot commented Jan 9, 2017

pwittrock commented Jan 10, 2017

juanvallejo commented Jan 10, 2017

k8s-ci-robot commented Jan 10, 2017

pwittrock commented Jan 10, 2017

pwittrock commented Jan 10, 2017

juanvallejo commented Jan 12, 2017

Choose a reason for hiding this comment

juanvallejo commented Jan 16, 2017

k8s-ci-robot commented Jan 16, 2017

juanvallejo commented Jan 16, 2017 • edited Loading

fabianofranz commented Jan 17, 2017

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

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

pwittrock commented Jan 18, 2017

juanvallejo commented Jan 18, 2017

pwittrock commented Jan 18, 2017

k8s-github-robot commented Jan 18, 2017

smarterclayton commented Mar 1, 2017

smarterclayton commented Mar 1, 2017

smarterclayton commented Mar 1, 2017

juanvallejo commented Jan 16, 2017 •

edited

Loading