Let kubectl run follow rules for `--` #13917

feihujiang · 2015-09-14T12:25:00Z

feihujiang · 2015-09-14T12:43:52Z

@smarterclayton @bgrant0607 Could you please review this PR?

k8s-bot · 2015-09-14T12:46:58Z

GCE e2e build/test failed for commit b7896da1ff484097fe9cf5a3c87b16dc25e2add3.

smarterclayton · 2015-09-14T14:49:22Z

pkg/kubectl/cmd/run.go

@@ -106,6 +107,23 @@ func Run(f *cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer, cmd *cob
 		return cmdutil.UsageError(cmd, "NAME is required for run")
 	}

+	// Let kubectl run follow rules for `--`
+	var commandAndCustomArgs []string


Hrm - this should be implemented by Cobra automatically. Maybe I misunderstand what this is trying to fix?

Nm, I reminded myself. There may be a more succint way to fix this

kubectl run -p POD should have zero args

kubectl exec POD should have zero args

kubectl run POD COMMAND should have 1 arg

kubectl run -p POD COMMAND should have 1 arg

If you use SetInterspersed(true) on the cobra.Command object, does that give you the correct behavior in all of these cases? (you might have to grab arg[0] for case 3 and shorten args to args[1:])

This commit doesn't fix these cases, it fixes #13004 . And run command hasn't -p operation.

k8s-github-robot · 2015-09-14T20:00:18Z

Labelling this PR as size/S

feihujiang · 2015-09-15T02:54:07Z

@smarterclayton This commit fixes #13004 you posted, that is "kubectl run --restart=Never --image=gcr.io/google_containers/busybox --command -- sleep 600
should either error (no name) or make up a name and set command to ["sleep", "600"]. It does neither, turning sleep into the pod name and making command 600. " I understood the bug is that -- isn't effective, -- couldn't seperate between the name and ... Did I get your point in #13004?

feihujiang · 2015-09-15T02:58:32Z

@k8s-bot test this please

k8s-bot · 2015-09-15T03:27:39Z

GCE e2e build/test failed for commit b7896da1ff484097fe9cf5a3c87b16dc25e2add3.

eparis · 2015-09-15T14:12:47Z

I'm trying to understand what we believe the 'syntax' is. These things work today:

kubectl run --image=busybox --command mycontainer echo "hello there"
kubectl run --image=busybox --command mycontainer -- echo "hello there"
kubectl run --image=busybox --command -- mycontainer echo "hello there"

But this patch will break 1 and 3...

bgrant0607 · 2015-09-16T06:06:40Z

--command was added by #12571, which was a 1.1-alpha release, so backward compatibility is not a concern. We should fix the command to be what we want it to be before we cut 1.1 -- so ASAP.

bgrant0607 · 2015-09-16T06:08:11Z

Could we just eliminate --command and just rely on --?

bgrant0607 · 2015-09-16T06:09:52Z

cc @kubernetes/kubectl

bgrant0607 · 2015-09-16T06:12:11Z

kubectl run mycontainer --image=busybox -- echo -e "\ahello there"

bgrant0607 · 2015-09-16T06:15:33Z

Nevermind about --command. I see that's a bool. That's fine.

kubectl run mycontainer --image=busybox --command -- echo -e "\ahello there"

feihujiang · 2015-09-16T06:18:55Z

@bgrant0607 --command isn't problem, the problem is that -- is useless now, we can't separate NAME and cmd arg1 ... argN, for example, If miss mycontainer, kubectl run --image=busybox --command -- echo -e "\ahello there", echo would become the name.

bgrant0607 · 2015-09-16T06:23:17Z

@eparis #13004 proposed that your (3) should not work. It's also fine by me if (1) does not work -- that we require --. But we should be consistent with exec:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/exec.go#L56

How does exec not suffer the same problem?

feihujiang · 2015-09-16T06:28:20Z

@bgrant0607 The exec has the same problem, I point it out in #13004

feihujiang · 2015-09-16T06:32:04Z

If we have the conclusion, I'd like to fix the same problem in exec.

feihujiang · 2015-09-16T06:54:27Z

@k8s-bot test this please

k8s-bot · 2015-09-16T07:41:02Z

GCE e2e build/test failed for commit b7896da1ff484097fe9cf5a3c87b16dc25e2add3.

feihujiang · 2015-09-16T07:58:10Z

@k8s-bot test this again please

k8s-bot · 2015-09-16T08:21:14Z

GCE e2e build/test failed for commit b7896da1ff484097fe9cf5a3c87b16dc25e2add3.

eparis · 2015-09-16T15:02:27Z

So git log (and other git like commands) don't allow 3) to work either. I don't mind if we 'break' that, the way that @feihujiang did.

I personally don't love breaking 1) and think it should continue to work. If the consensus is to force -- all the time I won't jump up and down and scream and scream and scream.

@feihujiang seems like it might be useful if cobra stored len(args) when -- was located. Let me write that quickly...

eparis · 2015-09-16T16:14:17Z

something like: eparis/pflag@b25fea9

eparis · 2015-09-16T16:32:53Z

along with eparis/cobra@edde52e

smarterclayton · 2015-09-16T16:49:06Z

I don't like forcing -- either

On Sep 16, 2015, at 11:02 AM, Eric Paris notifications@github.com wrote:

So git log (and other git like commands) don't allow 3) to work either. I
don't mind if we 'break' that, the way that @feihujiang
https://github.com/feihujiang did.

I personally don't love breaking 1) and think it should continue to work.
If the consensus is to force -- all the time I won't jump up and down and
scream and scream and scream.

@feihujiang https://github.com/feihujiang seems like it might be useful
if cobra stored len(args) when -- was located. Let me write that quickly...

—
Reply to this email directly or view it on GitHub
#13917 (comment)
.

smarterclayton · 2015-11-18T01:35:10Z

Sorry for the assign spam, bad github UI on mobile.

LGTM

k8s-github-robot · 2015-11-18T01:43:25Z

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

k8s-bot · 2015-11-18T01:55:49Z

GCE e2e build/test failed for commit d1c47c0e79bacc276ad5717dc009df1644fcb4cc.

feihujiang · 2015-11-18T02:53:01Z

@k8s-bot test this please

k8s-bot · 2015-11-18T03:19:08Z

GCE e2e test build/test passed for commit d1c47c0e79bacc276ad5717dc009df1644fcb4cc.

feihujiang · 2015-11-18T06:15:08Z

@k8s-bot test this please

feihujiang · 2015-11-18T06:38:05Z

I tested this PR locally, and all tests of unit/integration passed. I don't know why Jenkins unit/integration failing. Who could post the result of Jenkins unit/integration for me? I couldn't get it. Thanks very much.

k8s-bot · 2015-11-18T06:50:39Z

GCE e2e build/test failed for commit d1c47c0e79bacc276ad5717dc009df1644fcb4cc.

feihujiang · 2015-11-18T06:53:26Z

I rebased, and found the error.

janetkuo · 2015-11-18T07:00:55Z

e2e failure:

22:44:38 Summarizing 1 Failure:
22:44:38 
22:44:38 [Fail] Probing container [It] with readiness probe should not be ready before initial delay and never restart [Conformance] 
22:44:38 /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/container_probe.go:57

unit/integration failure:

22:27:30 FAIL   k8s.io/kubernetes/pkg/kubectl/cmd [build failed]
...
22:30:48 !!! Error in ./hack/test-go.sh:239
22:30:48   'return ${rc}' exited with status 2
22:30:48 Call stack:
22:30:48   1: ./hack/test-go.sh:239 main(...)
22:30:48 Exiting with status 1

k8s-github-robot · 2015-11-18T07:10:20Z

PR changed after LGTM, removing LGTM.

k8s-bot · 2015-11-18T07:31:05Z

GCE e2e test build/test passed for commit 4d2333b.

feihujiang · 2015-11-18T07:44:31Z

@smarterclayton I rebased this PR (use fake.CreateHTTPClient), please review again. @janetkuo thanks.

feihujiang · 2015-11-20T02:12:03Z

@smarterclayton Could you please review this PR, this PR is delayed a lot. Thanks!

k8s-github-robot · 2015-11-20T07:42:15Z

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

k8s-bot · 2015-11-20T08:12:38Z

GCE e2e test build/test passed for commit 4d2333b.

k8s-github-robot · 2015-11-20T08:13:17Z

Automatic merge from submit-queue

Auto commit by PR queue bot

* Removes a todo item from the list * Test coverage provided by kubernetes/kubernetes#13917 * closes openshift#8620

googlebot added the cla: yes label Sep 14, 2015

smarterclayton reviewed Sep 14, 2015
View reviewed changes

j3ffml assigned smarterclayton Sep 14, 2015

j3ffml added the area/kubectl label Sep 14, 2015

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 14, 2015

eparis mentioned this pull request Sep 15, 2015

kubectl run doesn't follow rules for -- #13004

Closed

Let kubectl run follow rules for --

4d2333b

feihujiang force-pushed the kubectlRunFollowRules branch from d1c47c0 to 4d2333b Compare November 18, 2015 07:02

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2015

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 20, 2015

k8s-github-robot pushed a commit that referenced this pull request Nov 20, 2015

Merge pull request #13917 from feihujiang/kubectlRunFollowRules

21095ea

Auto commit by PR queue bot

k8s-github-robot merged commit 21095ea into kubernetes:master Nov 20, 2015

feihujiang deleted the kubectlRunFollowRules branch November 24, 2015 07:03

0xmichalis mentioned this pull request May 3, 2016

add oc create dc openshift/origin#8712

Merged

jtslear added a commit to jtslear/origin that referenced this pull request Oct 3, 2016

Updates oc run examples

1cf275e

* Removes a todo item from the list * Test coverage provided by kubernetes/kubernetes#13917 * closes openshift#8620

This was referenced Oct 3, 2016

Updates oc run examples jtslear/origin#1

Closed

Updates oc run examples openshift/origin#11188

Merged

jtslear added a commit to jtslear/origin that referenced this pull request Oct 3, 2016

Updates oc run examples

095b906

* Removes a todo item from the list * Test coverage provided by kubernetes/kubernetes#13917 * closes openshift#8620

jtslear added a commit to jtslear/origin that referenced this pull request Oct 4, 2016

Updates oc run examples

d3851ba

* Removes a todo item from the list * Test coverage provided by kubernetes/kubernetes#13917 * closes openshift#8620

Let kubectl run follow rules for -- #13917

Let kubectl run follow rules for -- #13917

Conversation

feihujiang commented Sep 14, 2015

feihujiang commented Sep 14, 2015

k8s-bot commented Sep 14, 2015

smarterclayton Sep 14, 2015

Choose a reason for hiding this comment

smarterclayton Sep 14, 2015

Choose a reason for hiding this comment

feihujiang Sep 15, 2015

Choose a reason for hiding this comment

k8s-github-robot commented Sep 14, 2015

feihujiang commented Sep 15, 2015

feihujiang commented Sep 15, 2015

k8s-bot commented Sep 15, 2015

eparis commented Sep 15, 2015

bgrant0607 commented Sep 16, 2015

bgrant0607 commented Sep 16, 2015

bgrant0607 commented Sep 16, 2015

bgrant0607 commented Sep 16, 2015

bgrant0607 commented Sep 16, 2015

feihujiang commented Sep 16, 2015

bgrant0607 commented Sep 16, 2015

feihujiang commented Sep 16, 2015

feihujiang commented Sep 16, 2015

feihujiang commented Sep 16, 2015

k8s-bot commented Sep 16, 2015

feihujiang commented Sep 16, 2015

k8s-bot commented Sep 16, 2015

eparis commented Sep 16, 2015

eparis commented Sep 16, 2015

eparis commented Sep 16, 2015

smarterclayton commented Sep 16, 2015

smarterclayton commented Nov 18, 2015

k8s-github-robot commented Nov 18, 2015

k8s-bot commented Nov 18, 2015

feihujiang commented Nov 18, 2015

k8s-bot commented Nov 18, 2015

feihujiang commented Nov 18, 2015

feihujiang commented Nov 18, 2015

k8s-bot commented Nov 18, 2015

feihujiang commented Nov 18, 2015

janetkuo commented Nov 18, 2015

k8s-github-robot commented Nov 18, 2015

k8s-bot commented Nov 18, 2015

feihujiang commented Nov 18, 2015

feihujiang commented Nov 20, 2015

k8s-github-robot commented Nov 20, 2015

k8s-bot commented Nov 20, 2015

k8s-github-robot commented Nov 20, 2015

Let kubectl run follow rules for `--` #13917

Let kubectl run follow rules for `--` #13917