Add support for ENV of the form: ENV name=value ... by duglin · Pull Request #8251 · moby/moby

duglin · 2014-09-26T02:51:45Z

still supports the old form: ENV name value.
Notes:

presence of "=" in the first word is the trigger to know if we're going the old vs new form
new form supports quoting and 's the same way the cmd line does

since shells support:
abc=123 def=456
I think supporting:
ENV abc=123 def=456
makes a lot of sense. This isn't just about doing some kind of batching (which is being worked on in other PRs), this aligns ENV with what shells supports.

Keeping old description for posterity:
I have mixed feelings about this. While it works and is kind of cool to be able to do:
ENV abc=def foo="feed the dog" hungry=eat\ a\ pizza late="go to bed 'now' I said"
and each key=value is parsed like you'd expect it to be on the command line, the ability to do more than
one ENV at a time really should be done under some kind of batching mechanism. But, of course,
that's not there yet and we're not sure when it will be. So, unlike the RUN command where we can
concat things together with &&'s, the ENV command doesn't have that option - not without this feature.

Closes #2333

Signed-off-by: Doug Davis [email protected]

SvenDowideit · 2014-09-26T03:06:06Z

docs/sources/reference/builder.md

how about the entire string after the first space will be treated as the <value>?

Changed per your suggestion.

SvenDowideit · 2014-09-26T03:08:57Z

@jamtur01 @fredlf @ostezer

essentially Docs LGTM, but some tweaks would be nice

SvenDowideit · 2014-09-26T04:20:44Z

docs/sources/reference/builder.md

I've just tested (using make docs) and these indented code sections need an extra line feed:

For example: ENV myName="John Doe" myDog=Rex\ The\ Dog and ENV myName John Doe ENV myDog Rex The Dog will yield the same net results in the final container, but the first form does it all in one layer.

SvenDowideit · 2014-09-26T04:21:07Z

Docs LGTM

ghost · 2014-09-26T11:33:25Z

docs/sources/reference/builder.md

needs to be <value> I reckon.

I assume you meant that I needed to add single-back-quotes around it, if so, done. If not, can you elaborate?

ghost · 2014-09-26T11:57:14Z

Docs LGTM.

jessfraz · 2014-09-26T20:54:47Z

I don't exactly understand why this is necessary though.

jessfraz · 2014-09-26T20:54:58Z

@erikh ?

erikh · 2014-09-26T21:20:02Z

I think solomon needs to weigh in on this before we can merge it.

As for the feature, it could reduce layers, so I’m all for it.

On Sep 26, 2014, at 1:55 PM, Jessie Frazelle [email protected] wrote:

@erikh ?

—
Reply to this email directly or view it on GitHub.

thaJeztah · 2014-09-26T21:22:20Z

If I understand this correctly, it would reduce the number of layers produced, so for that, I'm +1, but possibly an improved builder could automatically optimise this? Multiple ENV instructions are more readable.

edit
Erik just beat me on that comment, lol

duglin · 2014-09-27T01:38:14Z

Yep, as I mentioned, I have mixed feelings and am ok either way we go - as long as we come to a decision and close the original issue.

To @thaJeztah's comment... yep, multiple ENVs are easier, but with this you can write:
ENV foo=bar
abc=def
dog=cat
which is similar to multiple ENVs, but all in one layer.

@shykes - any thoughts on this one?

thaJeztah · 2014-09-27T10:04:16Z

multiple ENVs are easier, but with this you can write ........ which is similar to multiple ENVs, but all in one layer.

Well, I think that points to the underlying problem. In the current implementation of the Dockerfile / builder, users have to "jump through hoops" to make docker do what they intended, or to optimise the results (reduce layers and/or reduce image-size).

In some cases, this results in "mangling" a perfectly readable Dockerfile into something that's hard to grasp, just for the sake of optimisation.

In short, even though I want this functionality (for optimisation), if there's a chance that the builder is able to optimise this automatically in the short future, I'm hesitant to introduce a new syntax as a stop-gap solution.

If such optimisation will not be possibly in the foreseeable future, OR this syntax has other advantages that I'm overseeing, then it's fine with me.

note
I know several issues exist to have more control over layers (caching/temporary build-contexts/build results), but I'm not sure there's an issue/proposal for (automatic) optimisation.

@erikh since you did the last major rewrite of the builder/parser, do you know if such issue already exists? Should one be created?

fredlf · 2014-09-29T18:44:31Z

Docs LGTM and thanks for the contribution.

crosbymichael · 2014-10-01T17:38:01Z

I think the design and UI looks good. We wanted the feature, we just did not like using the json syntax for it.

duglin · 2014-10-02T13:39:16Z

How do we decide if we want to move forward with this?

duglin · 2014-10-21T16:32:46Z

@crosbymichael thoughts?

erikh · 2014-10-21T17:18:35Z

I don't object to it; we're kicking out a 1.3.1 probably today with some builder fixes. Can we wait until after that?

erikh · 2014-10-21T17:19:52Z

Oh and this needs a rebase.

duglin · 2014-10-21T19:06:51Z

@erikh - rebase is done!

erikh · 2014-10-21T19:38:00Z

Thanks Doug. Ping me on IRC if I don’t get to this in the next few days, please.

On Oct 21, 2014, at 12:06 PM, Doug Davis [email protected] wrote:

@erikh https://github.com/erikh - rebase is done!

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

erikh · 2014-10-25T02:20:15Z

So, looking at this again, I would like to ask for a few things:

Use constants for PHASE, (an enum, see go's iota keyword) with a switch statement for handling the phases. Ideally, we could break these phases into their own, functional routines, but I'm not sure how feasible that is.
Handle the AST generation differently; set an even number of args == key and value, not parsing the = in a second place. This should result in very little code change to the evaluator's env dispatcher, I think, and result in a less coupled feature.

jessfraz · 2014-11-18T02:43:40Z

integration-cli/docker_cli_build_test.go

erikh · 2014-11-18T22:01:05Z

We need one more maintainer LGTM -- that means @tiborvass or @crosbymichael

duglin · 2014-11-19T03:20:46Z

@tiborvass @crosbymichael any comments?

milosgajdos · 2014-11-19T16:07:24Z

It would be nice to have this available on cli arguments not just within a Dockerfile. At the moment when I set an env variable as FOO="bar" Docker carries the quotes (single and double) inside the container

duglin · 2014-11-19T16:47:08Z

@milosgajdos83 normally the cmd line processing would do the right thing. For me I don't see quotes:

$ docker run -ti -e foo="bar" ubuntu env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=59fb35fdf2e7
TERM=xterm
foo=bar
HOME=/root

erikh · 2014-11-19T18:11:15Z

What shells are you two using?

-Erik

On Nov 19, 2014, at 8:47 AM, Doug Davis [email protected] wrote:

@milosgajdos83 https://github.com/milosgajdos83 normally the cmd line processing would do the right thing. For me I don't see quotes:

docker run -ti -e foo="bar" ubuntu env

PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=59fb35fdf2e7
TERM=xterm
foo=bar
HOME=/root

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

duglin · 2014-11-19T18:13:39Z

I get the same results from bash and sh.

erikh · 2014-11-19T19:05:47Z

Same result on zsh 5 here.

On Nov 19, 2014, at 10:13 AM, Doug Davis [email protected] wrote:

I get the same results from bash and sh.

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

tiborvass · 2014-11-19T23:58:25Z

docs/sources/reference/builder.md

maybe add a third example with ENV <key1>=<value1> <key2>=<value2> ?

line 357 isn't good enough?

It's good as well, but since this is right after the ENV title, it seems to me, it's a general template of what's possible with the keyword. Line 357 goes into even more detail with quotes. Anyway, it was just a suggestion. I guess docs people are better suited to answer this.

/cc @SvenDowideit @fredlf @jamtur01

actually I was about to add it (just for you :-) ) but then I realized, isn't the ... in there good enough since it means "repeat the last thing" ? If I added:
ENV = <key2=value2> ...
people might wonder what the difference is between that line and the previous one. And are we really saying we have a special syntax where we specify two or more and not just one or more?

At least my interpretation of that chunk of docs is that its not a sample but more like a pseudo-schema of what's expected. But I could be wrong.

Oh yeah maybe that's sufficient then :) If we don't hear from the docs guys about this issue, then let's just leave it.

duglin · 2014-11-20T01:27:28Z

ok I think I got all of @tiborvass 's suggested changes pushed now.

still supports the old form: ENV name value Also, fixed an issue with the parser where it would ignore lines at the end of the Dockerfile that ended with \ Closes moby#2333 Signed-off-by: Doug Davis <[email protected]>

crosbymichael · 2014-11-20T21:12:21Z

LGTM

Add support for ENV of the form: ENV name=value ...

duglin force-pushed the Issue2333 branch from 53f41aa to 1cc6909 Compare September 26, 2014 02:57

SvenDowideit reviewed Sep 26, 2014
View reviewed changes

duglin force-pushed the Issue2333 branch from 1cc6909 to a6984dc Compare September 26, 2014 03:12

SvenDowideit reviewed Sep 26, 2014
View reviewed changes

ghost reviewed Sep 26, 2014
View reviewed changes

duglin force-pushed the Issue2333 branch from a6984dc to cb6f428 Compare September 26, 2014 11:49

jamtur01 added the /project/doc label Sep 27, 2014

erikh self-assigned this Oct 21, 2014

duglin force-pushed the Issue2333 branch from cb6f428 to 2160757 Compare October 21, 2014 19:06

duglin mentioned this pull request Oct 25, 2014

Add support file name with whitespace for ADD and COPY command #8406

Closed

jessfraz added this to the 1.4 milestone Nov 18, 2014

jessfraz reviewed Nov 18, 2014
View reviewed changes

integration-cli/docker_cli_build_test.go

Copy link

Contributor

jessfraz Nov 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👯

duglin force-pushed the Issue2333 branch from dba84cc to 89a9f40 Compare November 18, 2014 02:48

duglin mentioned this pull request Nov 19, 2014

Shell friendly environment files #9240

Closed

tiborvass reviewed Nov 19, 2014
View reviewed changes

tiborvass added the UX label Nov 20, 2014

duglin force-pushed the Issue2333 branch from 89a9f40 to 15a7094 Compare November 20, 2014 01:26

Add support for ENV of the form: ENV name=value ...

1314e15

still supports the old form: ENV name value Also, fixed an issue with the parser where it would ignore lines at the end of the Dockerfile that ended with \ Closes moby#2333 Signed-off-by: Doug Davis <[email protected]>

duglin force-pushed the Issue2333 branch from 15a7094 to 1314e15 Compare November 20, 2014 17:54

crosbymichael added a commit that referenced this pull request Nov 20, 2014

Merge pull request #8251 from duglin/Issue2333

58b6f31

Add support for ENV of the form: ENV name=value ...

crosbymichael merged commit 58b6f31 into moby:master Nov 20, 2014

duglin deleted the Issue2333 branch November 21, 2014 20:11

duglin unassigned erikh Feb 5, 2015

thaJeztah added area/builder Build impact/dockerfile kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. labels Dec 2, 2019

Conversation

duglin commented Sep 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SvenDowideit commented Sep 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SvenDowideit commented Sep 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 26, 2014

Uh oh!

jessfraz commented Sep 26, 2014

Uh oh!

jessfraz commented Sep 26, 2014

Uh oh!

erikh commented Sep 26, 2014

Uh oh!

thaJeztah commented Sep 26, 2014

Uh oh!

duglin commented Sep 27, 2014

Uh oh!

thaJeztah commented Sep 27, 2014

Uh oh!

fredlf commented Sep 29, 2014

Uh oh!

crosbymichael commented Oct 1, 2014

Uh oh!

duglin commented Oct 2, 2014

Uh oh!

duglin commented Oct 21, 2014

Uh oh!

erikh commented Oct 21, 2014

Uh oh!

erikh commented Oct 21, 2014

Uh oh!

duglin commented Oct 21, 2014

Uh oh!

erikh commented Oct 21, 2014

Uh oh!

erikh commented Oct 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikh commented Nov 18, 2014

Uh oh!

duglin commented Nov 19, 2014

Uh oh!

milosgajdos commented Nov 19, 2014

Uh oh!

duglin commented Nov 19, 2014

Uh oh!

erikh commented Nov 19, 2014

Uh oh!

duglin commented Nov 19, 2014

Uh oh!

erikh commented Nov 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Nov 20, 2014