Fixes get --show-all #18165

fabianofranz · 2015-12-03T19:48:37Z

Fixes showIfTerminating in resource printer to make sure it only skips terminating pods and detaches it from the showAll check.

Worth to mention that both kubectl get pods and kubectl get pod <name> print objects individually, so printPodList is never called by the kubectl get command making --show-all=true|false do nothing in the previous logic.

k8s-github-robot · 2015-12-03T19:51:39Z

Labelling this PR as size/M

k8s-bot · 2015-12-03T20:35:05Z

GCE e2e build/test failed for commit 51c0d89f4a91ce9885150846cc7cffbfa2a25df0.

k8s-bot · 2015-12-03T20:42:40Z

GCE e2e build/test failed for commit ec1811b4adf4d0a87a0ee002c09ccd910bd51016.

janetkuo · 2015-12-03T20:52:30Z

pkg/kubectl/resource_printer.go

@@ -600,15 +641,6 @@ func printPodBase(pod *api.Pod, w io.Writer, withNamespace bool, wide bool, show
 	return err
 }

-func printPodList(podList *api.PodList, w io.Writer, withNamespace bool, wide bool, showAll bool, columnLabels []string) error {


Are these two functions moved without code changes? It might be better to keep them where they were, to make it easier to keep track of code changes.

They were moved without changes, the PrintObj method of HumanReadablePrinter is a little misplaced. Reverted.

k8s-github-robot · 2015-12-03T23:42:49Z

Labelling this PR as size/XS

k8s-bot · 2015-12-04T00:13:53Z

GCE e2e test build/test passed for commit d4939fba8daf076a5a8cd08489743346d206323e.

feihujiang · 2015-12-04T03:19:45Z

No, don't change the logic. Using showIfTerminating is to solve the problem that couldn't get the terminated pod by name. kubectl get pod should retrieve the pod regardless of its state. kubectl get pods would hide all terminated pods. If showIfTerminating is a little confusing, make the variable "showWhateverPodPhase" or other meaningful variable.

feihujiang · 2015-12-04T03:23:12Z

By default (--show-all=false), kubectl get pods would hide all terminated pods. When --show-all=true, kubectl get pods would show all pods.

fabianofranz · 2015-12-04T17:53:10Z

@feihujiang showIfTerminating is useless today for the get command because it's always true in any of the scenarios below. It happens because both get list and get by name call printPod, never printPodList.

# showIfTerminating is *always* true here

$ kubectl get pods
$ kubectl get pods -a
$ kubectl get pod <name>
$ kubectl get pod <name> -a

In result, the check in line 545 will never be fulfilled, making showAll (and in result --show-all=true|false) do nothing. This PR fixes --show-all to make it actually perform the filter.

feihujiang · 2015-12-05T04:10:33Z

@fabianofranz The problem is, change in this commit make $ kubectl get pod <name> couldn't get the terminated pod (pod.Status.Phase equal to api.PodSucceeded or equal to api.PodFailed) directly. And I think a good user experience is that kubectl get pod <name> should retrieve the pod regardless of its state.

janetkuo · 2015-12-09T01:59:53Z

We changed kubectl get code to always flatten podList first before printing it, in order to support kubectl get -f <list-type-resource>. Now getting a list of pods only calls printPod, but not printPodList.

We should find a new way to hide terminated pods by default if not getting the pod by its name, and show all pods when --show-all=true.

feihujiang · 2015-12-09T02:34:17Z

Yes, should find a new way to hide terminated pods.
Not getting the pod by its name only have the one method $ kubectl get pods.
Other methods are all getting the pod by its name, such as, $ kubectl get pods <name>, $kubectl get -f <list-type-resource>. @janetkuo If I'm wrong, please correct me.

fabianofranz · 2015-12-09T14:05:49Z

What about removing showIfTerminating and always rely on showAll - e.g. in the get command, if it's getting the resource by name then we force showAll to be true, which will make sure the named resource is always shown. If it sounds like a solution then I'll update this PR.

janetkuo · 2015-12-09T20:30:10Z

@fabianofranz sounds good! And as @feihujiang mentioned, we should hide terminated pods when kubectl get pods, but don't hide them when kubectl get pods <pod name> or kubectl get -f <file>.

k8s-github-robot · 2015-12-10T20:22:12Z

Labelling this PR as size/M

fabianofranz · 2015-12-10T20:24:27Z

Ok, changed this to remove showIfTerminating and always rely on showAll, which is forced to true when getting the resource by name or filename. PTAL

feihujiang · 2015-12-11T02:50:35Z

LGTM-- my comment is optional.

feihujiang · 2015-12-11T02:51:00Z

pkg/kubectl/resource/builder.go

+// HasNames returns true if the provided args contain resource names
+func HasNames(args []string) bool {
+	args = normalizeMultipleResourcesArgs(args)
+	hasCombinedTypes, _ := hasCombinedTypeArgs(args)


Does this function need to return the error, so that we can handle the error as early as possible?

We should check the returned error here.

feihujiang · 2015-12-11T07:47:40Z

Adding a test for this function in test-cmd.sh is better. :)

fabianofranz · 2015-12-11T14:54:33Z

@feihujiang Yeah, I'll add a test-cmd.sh test. Would you suggest an image we could use to check the Succeeded/Failed phase? I don't think there is something like that in test-cmd.sh yet.

feihujiang · 2015-12-11T16:40:18Z

We couldn't test this in test-cmd.sh, because Succeeded/Failed are the phase of the lifecycle of a pod, so pod couldn't turn into Succeeded or Failed immediately when we create it. However tests in test-cmd.sh are just for simple scenarios.

feihujiang · 2015-12-11T16:43:21Z

Add unit test is ok. :)

k8s-github-robot · 2015-12-11T19:12:33Z

Labelling this PR as size/L

fabianofranz · 2015-12-11T19:23:42Z

The printer already has some tests, so I added some for the flag enforcing and HasNames. PTAL

k8s-bot · 2015-12-11T19:44:20Z

GCE e2e test build/test passed for commit 37a83a0785ef0f07c3f36818ef0676f3018e5c69.

feihujiang · 2015-12-14T08:45:22Z

LGTM

fabianofranz · 2015-12-14T17:55:05Z

@janetkuo LGTMed, up to you to tag this. tks everyone!

janetkuo · 2015-12-17T05:56:27Z

pkg/kubectl/resource/builder_test.go

+			args:            []string{"rc/foo", "rc/bar", "rc/zee"},
+			expectedHasName: true,
+		},
+	}


add "" and invalid test cases such as "rc/foo", "bar"

janetkuo · 2015-12-17T06:05:43Z

Mostly LG other than nits. Thanks!

k8s-bot · 2015-12-17T21:12:28Z

GCE e2e test build/test passed for commit 717896e.

fabianofranz · 2015-12-17T21:14:52Z

@janetkuo comments addressed.

janetkuo · 2015-12-18T02:54:30Z

Thanks! LGTM.

k8s-github-robot · 2015-12-19T03:15:06Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-12-19T03:50:53Z

GCE e2e test build/test passed for commit 717896e.

k8s-github-robot · 2015-12-19T04:01:20Z

Automatic merge from submit-queue

Auto commit by PR queue bot

Automatic merge from submit-queue (batch tested with PRs 18129, 18152, 17403, 18020, 18165). UPSTREAM: 57422: Rework method of updating atomic-updated data volumes **What this PR does / why we need it**: This is a backport of kubernetes#57422 This change affects the way that secret, configmap, downwardAPI and projected volumes (which all use the same underlying code) implement their data update functionality. * Instead of creating a subdirectory hierarchy that will contain symlinks to each actual data file, create only symlinks to items in the root of the volume, whether they be files or directories. * Rather than comparing the user-visible data directory to see if an update is needed, compare with the current version of the data directory. * Fix data dir timestamp format year * Create `..data` symlink even when a data volume has no data so consumers can have simplified update watch logic. **Which issue(s) this PR fixes** This addresses https://bugzilla.redhat.com/show_bug.cgi?id=1516569 and https://bugzilla.redhat.com/show_bug.cgi?id=1430322 **Special notes for your reviewer**: **Release note**: ```release-note Allow volumes to be mounted beneath secret volumes [1430322] [1516569] ``` Origin-commit: 91d864d470581d67e38bbe3fe3b967aeea3feded

googlebot added the cla: yes label Dec 3, 2015

k8s-github-robot assigned bgrant0607 Dec 3, 2015

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 3, 2015

bgrant0607 assigned janetkuo and unassigned bgrant0607 Dec 3, 2015

fabianofranz force-pushed the fixes_get_show_all branch from 51c0d89 to ec1811b Compare December 3, 2015 20:31

janetkuo reviewed Dec 3, 2015
View reviewed changes

fabianofranz force-pushed the fixes_get_show_all branch from ec1811b to d4939fb Compare December 3, 2015 23:36

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

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2015

fabianofranz force-pushed the fixes_get_show_all branch from d4939fb to eabb9f2 Compare December 10, 2015 20:21

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 10, 2015

feihujiang reviewed Dec 11, 2015
View reviewed changes

fabianofranz force-pushed the fixes_get_show_all branch from eabb9f2 to 37a83a0 Compare December 11, 2015 19:11

k8s-github-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 Dec 11, 2015

janetkuo reviewed Dec 17, 2015
View reviewed changes

Fixes get --show-all

717896e

fabianofranz force-pushed the fixes_get_show_all branch from 37a83a0 to 717896e Compare December 17, 2015 20:36

janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2015

k8s-github-robot pushed a commit that referenced this pull request Dec 19, 2015

Merge pull request #18165 from fabianofranz/fixes_get_show_all

00c7541

Auto commit by PR queue bot

k8s-github-robot merged commit 00c7541 into kubernetes:master Dec 19, 2015

Fixes get --show-all #18165

Fixes get --show-all #18165

Conversation

fabianofranz commented Dec 3, 2015

k8s-github-robot commented Dec 3, 2015

k8s-bot commented Dec 3, 2015

k8s-bot commented Dec 3, 2015

janetkuo Dec 3, 2015

Choose a reason for hiding this comment

fabianofranz Dec 3, 2015

Choose a reason for hiding this comment

k8s-github-robot commented Dec 3, 2015

k8s-bot commented Dec 4, 2015

feihujiang commented Dec 4, 2015

feihujiang commented Dec 4, 2015

fabianofranz commented Dec 4, 2015

feihujiang commented Dec 5, 2015

janetkuo commented Dec 9, 2015

feihujiang commented Dec 9, 2015

fabianofranz commented Dec 9, 2015

janetkuo commented Dec 9, 2015

k8s-github-robot commented Dec 10, 2015

fabianofranz commented Dec 10, 2015

feihujiang commented Dec 11, 2015

feihujiang Dec 11, 2015

Choose a reason for hiding this comment

janetkuo Dec 17, 2015

Choose a reason for hiding this comment

feihujiang commented Dec 11, 2015

fabianofranz commented Dec 11, 2015

feihujiang commented Dec 11, 2015

feihujiang commented Dec 11, 2015

k8s-github-robot commented Dec 11, 2015

fabianofranz commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

feihujiang commented Dec 14, 2015

fabianofranz commented Dec 14, 2015

janetkuo Dec 17, 2015

Choose a reason for hiding this comment

fabianofranz Dec 17, 2015

Choose a reason for hiding this comment

janetkuo commented Dec 17, 2015

k8s-bot commented Dec 17, 2015

fabianofranz commented Dec 17, 2015

janetkuo commented Dec 18, 2015

k8s-github-robot commented Dec 19, 2015

k8s-bot commented Dec 19, 2015

k8s-github-robot commented Dec 19, 2015