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

Support shell syntax #130

Closed
drallgood opened this issue Mar 19, 2015 · 14 comments
Closed

Support shell syntax #130

drallgood opened this issue Mar 19, 2015 · 14 comments

Comments

@drallgood
Copy link

Right now, the CMD is specified in exec form no matter what.
This means, that you can't really use variables in you command (adding sh -cto the beginning doesn't always work).

It would be nice if you could choose between shell and exec form.

@akobiakov
Copy link

Agree with @drallgood: current way of parsing the "command" field looks a bit odd and counterintuitive for a docker user. I'd prefer a syntax which looks similar to official docker forms for CMD (shell/exec). Something like:

<command>
    <shell>java -jar /opt/demo/server.jar</shell>
</command>

and

<command>
    <exec>java</exec>
    <params>
        <param>-jar</param>
        <param>/opt/demo/server.jar</param>
    </params>
</command>

What do you think, guys?

@drallgood
Copy link
Author

I like the first one better. It's easier to create/read.

Implementation-wise, the easiest solution would be to just check for "sh -c" in the command and use the shell-syntax in case it's found.

@rhuss
Copy link
Collaborator

rhuss commented Mar 19, 2015

I like @akobiakov suggestion, since it is more explicit. I always hesitate a bit trying to be too "clever" (here: changing the Dockerfile syntax depending on the value a user gives). Also, having dedicate param tags would make it easier to provide args with spaces (don't know how well quoting works within Dockerfiles).

I would introduce this for 0.12.0 since it requires a (slight) configuration syntax change.

@drallgood
Copy link
Author

Awesome! :)

@rhuss
Copy link
Collaborator

rhuss commented Mar 19, 2015

Well, if anyone wants to work on that, it would be awesome :) I'm happy for each PR since I still have quite some other stuff to work on for 0.11.3 ;-) Please see this document for how to contribute (but you probably already know this). And please let us know in this issue if some of you starts to work on this (in order to avoid duplicate efforts).

@akobiakov
Copy link

Due to a bug in dockerfile parser we have to use a shell format which is currently not supported by the plugin, so an external Dockerfile is the only workaround for now.
Therefore, I'd be glad to undertake this enhancement.

@rhuss
Copy link
Collaborator

rhuss commented Mar 19, 2015

Perfect, simply submit a PR when you are done, I will happily review and apply it ;-)

Thanks!

@alan-czajkowski
Copy link
Contributor

I've reported a similar issue here: #149

I describe in detail why sh -c does not work (according to Docker documentation), and how to make it work -- which the plugin does not support.

@alan-czajkowski
Copy link
Contributor

is there an ETA on this? would be nice to have this feature (as I don't like the Spotify Maven Docker plugin)

@akobiakov
Copy link

@alan-czajkowski We're working on it. My colleague is going to create a PR tomorrow.

@akobiakov
Copy link

@rhuss
Copy link
Collaborator

rhuss commented May 7, 2015

Cool ;-) 'hope that I can review and merge it in ASAP, hopefully this week.

@alan-czajkowski
Copy link
Contributor

awesome work guys, once 0.12.0 comes out then i will test it out and #149 may become an obsolete issue

@rhuss rhuss added this to the 0.12.0 milestone May 18, 2015
rhuss added a commit that referenced this issue Jun 5, 2015
rhuss added a commit that referenced this issue Jun 5, 2015
since this maps better to Dockerfile syntax. #130
@rhuss
Copy link
Collaborator

rhuss commented Jun 14, 2015

Yeah, finally in in 0.12.0. 'will be released this week, stay tuned .... ;-)

Thanks again for your support and you patience ...

@rhuss rhuss added the fixed label Jun 14, 2015
rhuss pushed a commit that referenced this issue Jun 14, 2015
introduced cmd and entrypoint with exec and shell form support.
the change is not backward compatible to the previous version.

Signed-off-by: Oleg Sigida <oleg.sigida@gmail.com>
rhuss added a commit that referenced this issue Jun 14, 2015
rhuss added a commit that referenced this issue Jun 14, 2015
since this maps better to Dockerfile syntax. #130
@rhuss rhuss closed this as completed Jun 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants