Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for cmd and args when creating a new service #604

Closed
duglin opened this issue Jan 10, 2020 · 11 comments · Fixed by #635
Closed

Add support for cmd and args when creating a new service #604

duglin opened this issue Jan 10, 2020 · 11 comments · Fixed by #635
Assignees
Labels
good first issue Denotes an issue ready for a new contributor. kind/feature New feature or request kind/proposal Issues or PRs related to proposals.
Milestone

Comments

@duglin
Copy link
Contributor

duglin commented Jan 10, 2020

In what area(s)?

Classifications:
/kind good-first-issue
/kind feature
/kind proposal

Describe the feature:

Knative, via yaml, supports specifying the cmd and args in the podSpec - kn should support this too since there are times when people may need to specify which exe in the image to run or what args to pass to the exe. Sometimes the default values in the image are not sufficient.

@duglin duglin added the kind/feature New feature or request label Jan 10, 2020
@knative-prow-robot knative-prow-robot added good first issue Denotes an issue ready for a new contributor. kind/proposal Issues or PRs related to proposals. labels Jan 10, 2020
@rhuss
Copy link
Contributor

rhuss commented Jan 30, 2020

Thanks this would be a very helpful addition (i hope tha cmd and args are part of the Knative contract ;-). I suggest to use the following additional options:

  • --arg myfirstarg --arg myarg2 ...
  • --command "sh script.sh" (instead of the canonical --command sh --command script.sh so it is easier to use. Typically one will have only a single binary to use for the command)

For kn service update myservice --command newcmd --arg first --arg second this should override the existing command and arg arrays.

@duglin
Copy link
Contributor Author

duglin commented Jan 30, 2020

Another option for the args is to make it the last thing on the command line, then they could just do: kn service create foo --image foo --command /myexe -- arg1 arg2 arg3
Having to specify --arg for each one is a lot of extra typing.

@rhuss
Copy link
Contributor

rhuss commented Jan 30, 2020

Well, that interfers with the name (imaging someone forgets the name then the first arg is taken as name). I'd like to reserve non-option slot explicitly for names/identifiers, not for rarely used features. Also, there is no support for detecting whether arguments come last or in between.

@duglin
Copy link
Contributor Author

duglin commented Jan 30, 2020

The -- is the signal for the args, I don't think it'll interfere with the name.
See: https://godoc.org/github.com/spf13/cobra#Command.ArgsLenAtDash

@rhuss
Copy link
Contributor

rhuss commented Jan 30, 2020

The -- is the signal for the args, I don't think it'll interfere with the name.

Yes, but then you could have to put the service name after the double dash, too, intermixing identifiers and args. Also, you would then require to use the dash. Imo not something easy to memorize.

Is it really that important and do you expect many args to make --arg special and mix names/identifier with this (imo relatively niche) feature and introduce a new concept ("double dash required" for adding args) ? Afaik know --, it used as optional feature to help the user to use flags as arguments (like in rm -- -f for removing a file -f). Not as a mandatory requirement.

@duglin
Copy link
Contributor Author

duglin commented Jan 30, 2020

I didn't follow your intermixing comment. But, my mind jumps to something like docker run an kubectl run (which does use --), where at some point the "rest of the cmd line args" are used as what's passed on to the container. Very similar to your rm -- -f example to me.

So, unless we can think of another set of cmd line args that might want to make sure of -- I don't think it's horrible to think of kn service create as really much different than kubectl run where you're asking for an image to be executed and "here are some extra args to pass in" - because that's really what's going on anyway.

I do agree, this isn't huge since I don't expect it to be used very often, but if -- goes unused then I'm not sure I see the argument for requiring --arg to be repeated and annoy those users.

@rhuss
Copy link
Contributor

rhuss commented Jan 30, 2020

Ah, I see now where you come from: kubectl run (and to some degree docker run but with docker run it really depends also on the order of options/arguments).

But have you tried this with kubectl :

kubectl run demo --generator=run-pod/v1 --image busybox --command -- echo hello world
kubectl run --generator=run-pod/v1 --image busybox --command -- demo echo hello world
kubectl run --command demo --generator=run-pod/v1 --image busybox -- echo hello world
kubectl run demo --generator=run-pod/v1 --image busybox echo hello world
kubectl run --generator=run-pod/v1 --image busybox --command demo echo hello -- world

all are the same, some of them more confusing than others. (btw, just can just drop the -- if you don't have to use a -something in your args)

All what matters here is the order of arguments (regardless of the --), and the first argument is the name of the pod, all others are arguments. You can put the arguments anywhere in between options, its just doesn't matter.

For Docker its even more confusing, as there will be an implicit -- added after you have provided the image name.

@rhuss
Copy link
Contributor

rhuss commented Jan 30, 2020

Tbh, I really would like to keep that convention: The only argument is the name.

(Remember the discussion about adding the image as an argument, because its not an 'option' but mandatory for kn service create ? We deliberately decided to be more verbose in favour of clearness over cleverness. In fact I would much prefer to use the image name as an argument then args to a command, as the image name is mandatory, args are not. But I'm fine with the decission we made at that time)

@navidshaikh
Copy link
Collaborator

/assign @dsimansk

@knative-prow-robot
Copy link
Contributor

@navidshaikh: GitHub didn't allow me to assign the following users: dsimansk.

Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @dsimansk

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.

@navidshaikh navidshaikh added this to the v0.13.0 milestone Feb 4, 2020
@dsimansk
Copy link
Contributor

dsimansk commented Feb 4, 2020

/assign @dsimansk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor. kind/feature New feature or request kind/proposal Issues or PRs related to proposals.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants