add `oc describe` help suggestion to cmds with `--container` option #10469

juanvallejo · 2016-08-16T22:04:14Z

Fixes #10283
UPSTREAM: kubernetes/kubernetes#30717
This is an alternate fix to reverted PR: #10360

Commands with the --container= option provide no suggestions to a user
on how to obtain a container's name from a pod.

This patch adds a suggestion to use oc describe <pod_name> as part of the output of oc exec <pod> <cmd> when <pod> has more than one container.

$ oc exec database-1-h9dl1 -- /bin/sh -c "echo test"

defaulting container name to ruby-helloworld-database, use 'oc describe database-1-h9dl1' to see all containers in this pod
test

cc @fabianofranz @smarterclayton

smarterclayton · 2016-08-16T22:07:52Z

Looking at the changes required here, this seems like something that should be on the flag. And it doesn't have to be that complex - even just --container NAME .... You can find the list of containers on a pod with the 'describe' command is enough.

In general on help messages if we have to add a bunch of abstractions it often isn't worth it.

juanvallejo · 2016-08-16T22:38:19Z

@smarterclayton Thanks for the feedback.

this seems like something that should be on the flag. And it doesn't have to be that complex - even just --container NAME .... You can find the list of containers on a pod with the 'describe' command is enough.

Done!

juanvallejo · 2016-08-17T14:24:01Z

[test]

smarterclayton · 2016-08-17T14:32:40Z

Much simpler, thanks

deads2k · 2016-08-17T17:06:37Z

My comment from the upstream pull:

You can run these command against things that aren't pods. Why would we give advice that is only valid for a pod? Also, that's a super long description.

Can you elaborate on reasoning? The help string as provided is super long, its incorrect, and it seems like most people who have multiple containers in a pod where the first one isn't the one they want would know what they have.

juanvallejo · 2016-08-17T17:34:34Z

@deads2k

The help string as provided is super long, its incorrect, and it seems like most people who have multiple containers in a pod where the first one isn't the one they want would know what they have.

My reasoning for this was that it might give new users a way to know how to get container names to pass to a command's --container flag.

You can run these command against things that aren't pods. Why would we give advice that is only valid for a pod?

Maybe it could be used in one of the usage examples instead? Also, so that it remains consistent with commands that take a resource and not specifically a pod as an argument, maybe it could be rephrased or removed from those?

I think another solution to this would be to inform the user how to list pod containers when new-app is run for example. What are your thoughts on this?

deads2k · 2016-08-17T17:38:17Z

My reasoning for this was that it might give new users a way to know how to get container names to pass to a command's --container flag.

Have you thought about why they get there? It's hard to get there without intentionally creating multiple containers, in which case you know what you're looking for. Having to specify a container without knowing about the container means that there are multiple containers in your podspec-y thing, that the most important one isn't first, and that you didn't define the podspecy thing.

juanvallejo · 2016-08-17T19:06:12Z

Having to specify a container without knowing about the container means that there are multiple containers in your podspec-y thing, that the most important one isn't first, and that you didn't define the podspecy thing.

That is a fair point.
I can close this PR, kubernetes/kubernetes#30717, and issue #10283 if that is okay

smarterclayton · 2016-08-17T19:26:34Z

In practice, people will use this flag only for things that have pods, so
it's not really incorrect to explain in more detail what is going on here.
Going into more detail on specific flags doesn't violate the intent.

On Wed, Aug 17, 2016 at 3:06 PM, Juan Vallejo [email protected]
wrote:

Having to specify a container without knowing about the container means
that there are multiple containers in your podspec-y thing, that the most
important one isn't first, and that you didn't define the podspecy thing.

That is a fair point.
I can close this PR, kubernetes/kubernetes#30717
kubernetes/kubernetes#30717, and issue #10283
#10283 if that is okay

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10469 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p2sUHU94RR4Ph2OfUnhGwRFyKyzeks5qg1umgaJpZM4Jl5fF
.

danmcp · 2016-08-17T19:55:32Z

@deads2k

Have you thought about why they get there?

User picks something from add project and creates it
User wants to exec/rsync/rsh/etc to a particular container from what they launched
User sees the --container option

How do they know how to find the container they are looking for?

deads2k · 2016-08-17T20:02:28Z

In practice, people will use this flag only for things that have pods, so
it's not really incorrect to explain in more detail what is going on here.
Going into more detail on specific flags doesn't violate the intent.

But practically, if I don't know about containers, I'll go through a flow like this:

create dc foo
oc exec dc foo (this will always do something, but lets say I read the help first and decide that an optional parameter I don't know about is important for me to specify)
read help and see describe pod
huh, what's a pod. Alright, let's find pods.
wait, why do I have more than one pod.
all the names are funny, which pod do I need.
realize I didn't need the parameter anyway because 90+% of DCs have one container (or most important first).

I'm not sure how you make a more helpful description, but when I run oc exec --help so that I can run oc exec dc/foo, telling me to find the correct pod doesn't seem like the best way to do it.

smarterclayton · 2016-08-17T20:25:37Z

Most of the places we have -c because the assumption is most of the time
they're dealing with pods. So the -c flag having something pod specific is
not incorrect (maybe you can broaden it to something beyond pods, like
"objects with pods") but i doubt that reads better.

On Wed, Aug 17, 2016 at 3:55 PM, Dan McPherson [email protected]
wrote:

@deads2k https://github.com/deads2k

Have you thought about why they get there?

User picks something from add project and creates it

User wants to exec/rsync/rsh/etc to a particular container from what
they launched

User sees the --container option

How do they know how to find the container they are looking for?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10469 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p3WbYmgIGe7M-BsdhGXRt7_ieBhJks5qg2c3gaJpZM4Jl5fF
.

fabianofranz · 2016-08-17T20:36:36Z

If we'd like to try a completely different approach here, one idea would be to make this line an actual Print (probably Stderr) instead of just log, and improve the message adding something that points to describe.

Since this is a runtime solution instead of just help, it would only be displayed if the pod really has more than one container. Something like

$ oc exec happy-pod-with-lotsa-containers date
 > defaulting container name to container1-of-42, use 'oc describe happy-pod-with-lotsa-containers' to see all containers in this pod
Qua Ago 17 17:33:25 BRT 2016

deads2k · 2016-08-18T12:33:54Z

If we'd like to try a completely different approach here, one idea would be to make this line an actual Print (probably Stderr) instead of just log, and improve the message adding something that points to describe.

I think I'd like that better. That would actually help a user find the container they need instead of trying to backtrack through all the layers.

juanvallejo · 2016-08-18T21:05:34Z

@fabianofranz Thanks for the feedback! I have gone ahead and replaced the glog in https://github.com/juanvallejo/origin/blob/55bfc496d26a6f7eedc6caf1a6337bd8c1430ddc/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/exec.go#L250 to print its message + the oc describe suggestion to stderr

juanvallejo · 2016-08-18T21:31:16Z

[test]

… names Fixes openshift#10283 Commands with the `--container=` option provide no suggestions to a user on how to obtain a container's name from a pod. This patch adds a suggestion to use `oc describe <pod_name>` as part of the output of `oc exec <pod> <cmd>` ``` $ oc exec database-1-h9dl1 -- /bin/sh -c "echo test" defaulting container name to ruby-helloworld-database, use 'oc describe database-1-h9dl1' to see all containers in this pod test ```

fabianofranz · 2016-08-19T21:03:59Z

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

@@ -226,7 +226,7 @@ Find more information at https://github.com/kubernetes/kubernetes.`,
 	cmds.AddCommand(NewCmdUncordon(f, out))

 	cmds.AddCommand(NewCmdAttach(f, in, out, err))
-	cmds.AddCommand(NewCmdExec(f, in, out, err))
+  cmds.AddCommand(NewCmdExec("kubectl", f, in, out, err))


Indentation looks a little weird here on Github but I'm going to assume it's correct since gofmt didn't catch this.

Thanks for catching this, the gofmt plugin I have did not seem to catch it either. Fixed!

fabianofranz · 2016-08-19T21:04:39Z

If everyone is ok with the approach, LGTM.

deads2k · 2016-08-22T11:50:11Z

If everyone is ok with the approach, LGTM.

tagged upstream (though it now needs a rebase), [merge]

openshift-bot · 2016-08-22T13:17:21Z

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

openshift-bot · 2016-08-22T14:42:23Z

Evaluated for origin test up to bc5afbb

openshift-bot · 2016-08-22T16:08:48Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8304/)

deads2k · 2016-08-22T17:46:26Z

[merge]

openshift-bot · 2016-08-22T17:47:23Z

Evaluated for origin merge up to bc5afbb

juanvallejo changed the title ~~Jvallejo add oc describe help suggestion~~ add oc describe help suggestion to cmds with --container option Aug 16, 2016

juanvallejo mentioned this pull request Aug 16, 2016

It isn't clear how to get a container name from oc exec #10283

Closed

juanvallejo force-pushed the jvallejo_add-oc-describe-help-suggestion branch from 06e997a to e4fa367 Compare August 16, 2016 22:37

juanvallejo force-pushed the jvallejo_add-oc-describe-help-suggestion branch from e4fa367 to 55bfc49 Compare August 16, 2016 22:48

juanvallejo force-pushed the jvallejo_add-oc-describe-help-suggestion branch 3 times, most recently from 5fc1ebf to 2595e56 Compare August 18, 2016 21:03

juanvallejo mentioned this pull request Aug 18, 2016

add suggestion to use describe to obtain container names kubernetes/kubernetes#30717

Merged

juanvallejo force-pushed the jvallejo_add-oc-describe-help-suggestion branch from 2595e56 to de2c10a Compare August 18, 2016 21:13

juanvallejo force-pushed the jvallejo_add-oc-describe-help-suggestion branch from de2c10a to 377d43d Compare August 19, 2016 16:29

fabianofranz reviewed Aug 19, 2016
View reviewed changes

add suggestion to use describe to obtain container names

bc5afbb

juanvallejo force-pushed the jvallejo_add-oc-describe-help-suggestion branch from 377d43d to bc5afbb Compare August 22, 2016 14:40

openshift-bot merged commit a1c9eb1 into openshift:master Aug 23, 2016

juanvallejo deleted the jvallejo_add-oc-describe-help-suggestion branch August 23, 2016 13:20

juanvallejo mentioned this pull request Oct 25, 2016

add suggestion to use describe to obtain container names #10360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `oc describe` help suggestion to cmds with `--container` option #10469

add `oc describe` help suggestion to cmds with `--container` option #10469

juanvallejo commented Aug 16, 2016 •

edited

Loading

smarterclayton commented Aug 16, 2016

juanvallejo commented Aug 16, 2016

juanvallejo commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

deads2k commented Aug 17, 2016

juanvallejo commented Aug 17, 2016

deads2k commented Aug 17, 2016

juanvallejo commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

danmcp commented Aug 17, 2016

deads2k commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

fabianofranz commented Aug 17, 2016

deads2k commented Aug 18, 2016

juanvallejo commented Aug 18, 2016

juanvallejo commented Aug 18, 2016

fabianofranz Aug 19, 2016

juanvallejo Aug 22, 2016

fabianofranz commented Aug 19, 2016

deads2k commented Aug 22, 2016

openshift-bot commented Aug 22, 2016 •

edited

Loading

openshift-bot commented Aug 22, 2016

openshift-bot commented Aug 22, 2016

deads2k commented Aug 22, 2016

openshift-bot commented Aug 22, 2016

add oc describe help suggestion to cmds with --container option #10469

add oc describe help suggestion to cmds with --container option #10469

Conversation

juanvallejo commented Aug 16, 2016 • edited Loading

smarterclayton commented Aug 16, 2016

juanvallejo commented Aug 16, 2016

juanvallejo commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

deads2k commented Aug 17, 2016

juanvallejo commented Aug 17, 2016

deads2k commented Aug 17, 2016

juanvallejo commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

danmcp commented Aug 17, 2016

deads2k commented Aug 17, 2016

smarterclayton commented Aug 17, 2016

fabianofranz commented Aug 17, 2016

deads2k commented Aug 18, 2016

juanvallejo commented Aug 18, 2016

juanvallejo commented Aug 18, 2016

fabianofranz Aug 19, 2016

Choose a reason for hiding this comment

juanvallejo Aug 22, 2016

Choose a reason for hiding this comment

fabianofranz commented Aug 19, 2016

deads2k commented Aug 22, 2016

openshift-bot commented Aug 22, 2016 • edited Loading

openshift-bot commented Aug 22, 2016

openshift-bot commented Aug 22, 2016

deads2k commented Aug 22, 2016

openshift-bot commented Aug 22, 2016

add `oc describe` help suggestion to cmds with `--container` option #10469

add `oc describe` help suggestion to cmds with `--container` option #10469

juanvallejo commented Aug 16, 2016 •

edited

Loading

openshift-bot commented Aug 22, 2016 •

edited

Loading