Perhaps a good example of craziness can be found in the busybox dockerfile that's used on windowsTP5 CI. https://github.com/jhowardmsft/busybox/blob/master/Dockerfile In the RUN, extra escaping is needed for cmd, conveniently the carat, making the actual command that's needed even more obfuscated. This PR would allow that dockerfile to be simplified, and far more readable to someone maintaining it. I'm sure there are countless other examples out there.

Isn't it just easier to use the json form? I know it is a bit more verbose to write, but it already achieves what you want, not running an extra shell and not requiring escaping (other than double quotes)?

Not in my experience. Ease of use trumps verbosity and readability IMO

This is not the first time there's been a similar discussion.... unfortunately buried in GitHub issues.

I'm sure there have, but I'm not aware of any which involve the Windows angle where it's a far more acute issue.

ping @docker/core-engine-api-maintainers we want to have a broader opinion on this one, so please give your +1 or -1. We'll revisit in a week or so to see if we move forward

+1, and agree with @justincormack that the JSON syntax is probably more consistent

+1! (maybe counts as 1/2 a vote)

@jfrazelle haha, should've sneaked into the hangout :)

We discussed this (again), and there were no objections from the people present in the maintainers meeting (hangout), so moved this to code review

Nah, I think that counts as at least 2 votes 😸

@thaJeztah, which release can get this change? 1.11.2?

@tarunlalwani no this is an API change, so it doesn't go into a patch release, so will go into 1.12.0

Erm, meant change in the Dockerfile, not API

@jhowardmsft @tianon @jfrazelle thinking about this feature some more; some questions;

• do we want only a single SHELL to be allowed in a Dockerfile? (at least, to start with)
• should SHELL be inherited from the FROM image? Having it inherit could be very confusing for users, because they don't know what shell is set in the base image.
• Could this feature be abused to run arbitrary code on each RUN? Is this a concern? Should we limit accepted values for SHELL for that reason?
• Which instructions are affected by SHELL? We need to be explicit about this; do ENTRYPOINT and CMD also use this? Do we want them to?

@thaJeztah, I have not been asked still would like to add my opinions :)

• do we want only a single SHELL to be allowed in a Dockerfile? (at least, to start with)

Multiple should be allowed and just overwrites the previous SHELL entry

• should SHELL be inherited from the FROM image? Having it inherit could be very confusing for users, because they don't know what shell is set in the base image.

  Inheritance should be allowed, that is the main reason for asking such a feature. But docker can print a warning "Using non-default shell /bin/bash -lc"

• Could this feature be abused to run arbitrary code on each RUN? Is this a concern? Should we limit accepted values for SHELL for that reason?

  Not sure on this, probably I would let others comment

• Which instructions are affected by SHELL? We need to be explicit about this; do ENTRYPOINT and CMD also use this? Do we want them to?

  SHELL should only impact the RUN command, which is a BUILD command and not touch ENTRYPOINT and CMD

• do we want only a single SHELL to be allowed in a Dockerfile? (at least, to start with)

Multiple SHELL, working like WORKDIR 😝. This way you could setup some magic shell while building the image and reset it to a sane default before the end of the build…

• should SHELL be inherited from the FROM image? Having it inherit could be very confusing for users, because they don't know what shell is set in the base image.

I would say yes too but I see the confusion coming too 😓. And combined with the next one (running arbitrary code) it might be safer for this to not inherit.

• Could this feature be abused to run arbitrary code on each RUN? Is this a concern? Should we limit accepted values for SHELL for that reason?

It definitely could but limiting accepted values is not gonna be good either. I'm definitely worried about that one though (and is almost a stopper for me 😓). With inherinting, to be safe, you would have to setup the shell to your sane default for each image you wanna build (which parent is not maintained by you) — it's gonna add noise and more not useful layers.

• Which instructions are affected by SHELL? We need to be explicit about this; do ENTRYPOINT and CMD also use this? Do we want them to?

Definitely only RUN.

Although I really like the idea and the hacking (in the good sence of it) it brings, I'm still a little bit meh / worried about having this in the Dockerfile(s).

I'm not so sure that SHELL shouldn't apply to ENTRYPOINT and CMD too since those wrapper cmds with "sh -c" too. Clearly on Windows "sh -c" isn't going to work for ENTRYPOINT/CMD so why wouldn't it apply there too?

And yes, I think it should be inherited and therefore should be kept in the image metadata.

Clearly on Windows "sh -c" isn't going to work for ENTRYPOINT/CMD so why wouldn't it apply there too?

On windows, it would default to CMD.exe

And yes, I think it should be inherited and therefore should be kept in the image metadata.

I'm really not sure about that; same as with the escape, we don't inherit that, because users wouldn't know what the escape character is. Similarly, they wouldn't know what the shell is in the commands they run.

Perhaps it's not a direct responsibility for Docker, but I'm kinda worried about inheriting this, not only because it could be exploited in nasty ways (setting the shell to a script that uploads env?), but also because of the confusion mentioned above

LGTM

LGTM once janky is happy

Quick question

There is a command like sh -c "command" and sh -l -c "command". In first case if I define SHELL as sh -c then how would the run command look like? In second case if I use SHELL as sh -l, how would the RUN command be written?

First one is same as default on Linux. RUN command. Second one would be RUN -c command

@jhowardmsft, How does one differentiate between sh -l -c "param" and sh -l "-c param" where SHELL is defined as sh -l?

This PR does not change that. You can use escaping in the JSON form to distinguish between those.

@jhowardmsft , Got it!

@thaJeztah Docs review time? 😄

Oh, we have two! Yes, moving, I'll give it a look later

LGTM, I wanted to do some other updates to this document, so I can address that quote while doing so

ping @vdemeester for review/merge

LGTM 🐠

🎉 thanks @jhowardmsft!

Woohoo! Thanks!!!