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

Patch to commit-change patch to add docker import support #9123

Merged
merged 8 commits into from Feb 24, 2015

Conversation

Projects
None yet
@rhatdan
Contributor

rhatdan commented Nov 12, 2014

pass --change changes to the import job

Docker-DCO-1.1-Signed-off-by: Dan Walsh dwalsh@redhat.com (github: rhatdan)

I have added a patch to #8765

Which implements docker import --change.

@rhatdan rhatdan changed the title from Patch to origin/commit-change patch to add docker import support to Patch to commit-change patch to add docker import support Nov 12, 2014

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Nov 13, 2014

Contributor

Docs LGTM - @fredlf @jamtur01

Contributor

SvenDowideit commented Nov 13, 2014

Docs LGTM - @fredlf @jamtur01

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Nov 13, 2014

Contributor

oh sneaky (copying from the original PR)

I think the docs need more info about side effects, or confirming that there are none - does the instruction run in the container, and so will persist if i docker start it again, or is the container commited, then a new one made based on that image, and the instruciton run in that temporary container..

Contributor

SvenDowideit commented Nov 13, 2014

oh sneaky (copying from the original PR)

I think the docs need more info about side effects, or confirming that there are none - does the instruction run in the container, and so will persist if i docker start it again, or is the container commited, then a new one made based on that image, and the instruciton run in that temporary container..

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Nov 13, 2014

Contributor

I think the docs need more info about side effects, or confirming that there are none
Not sure what you mean about side effects. Basically what happens in a docker --commit or docker --import is you end up with an image and a json file containing any additional instructions given on --change line.

It will have the same output as if you had done your changes with a Dockerfile.

docker run -ti fedora /bin/sh
yum -y update
exit
docker commit --change "ENV foo bar" --change CMD /bin/init

Should give the same output as

cat Dockerfile
FROM fedora
RUN yum -y update
ENV foo bar
CMD /bin/init

does the instruction run in the container, and so will persist if i docker start it again, or is the container commited, then a new one made based on that image,
The commit/import and the change happen at the same layer their is no special commands, these are just adding data to the json file about the layer.

Contributor

rhatdan commented Nov 13, 2014

I think the docs need more info about side effects, or confirming that there are none
Not sure what you mean about side effects. Basically what happens in a docker --commit or docker --import is you end up with an image and a json file containing any additional instructions given on --change line.

It will have the same output as if you had done your changes with a Dockerfile.

docker run -ti fedora /bin/sh
yum -y update
exit
docker commit --change "ENV foo bar" --change CMD /bin/init

Should give the same output as

cat Dockerfile
FROM fedora
RUN yum -y update
ENV foo bar
CMD /bin/init

does the instruction run in the container, and so will persist if i docker start it again, or is the container commited, then a new one made based on that image,
The commit/import and the change happen at the same layer their is no special commands, these are just adding data to the json file about the layer.

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Nov 14, 2014

Contributor

mmm, @rhatdan that needs to be explained in the docs. So to confirm, each --change flag creates a new image layer?

ie, it needs to be clear that it does not modify the original container, but actually commits it, then applies each --change in turn in a new layer, tagging the final one...

Contributor

SvenDowideit commented Nov 14, 2014

mmm, @rhatdan that needs to be explained in the docs. So to confirm, each --change flag creates a new image layer?

ie, it needs to be clear that it does not modify the original container, but actually commits it, then applies each --change in turn in a new layer, tagging the final one...

@dqminh

This comment has been minimized.

Show comment
Hide comment
@dqminh

dqminh Nov 14, 2014

Contributor

@SvenDowideit iirc, all --change will be combined together and create only 1 layer ( that's how docker commit --change does it ).

@crosbymichael i think import will still having the same problem as commit because the current path is still passing --change data as URL params as we discussed before right ?

Contributor

dqminh commented Nov 14, 2014

@SvenDowideit iirc, all --change will be combined together and create only 1 layer ( that's how docker commit --change does it ).

@crosbymichael i think import will still having the same problem as commit because the current path is still passing --change data as URL params as we discussed before right ?

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Nov 14, 2014

Contributor

@dqminh @crosbymichael Well the import code is basically a cut and paste of the commit code. What is the problem with passing the --change data in the URL?

Contributor

rhatdan commented Nov 14, 2014

@dqminh @crosbymichael Well the import code is basically a cut and paste of the commit code. What is the problem with passing the --change data in the URL?

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Nov 14, 2014

Contributor

@dqminh don't tell me :) it needs to be in the documentation!

Contributor

SvenDowideit commented Nov 14, 2014

@dqminh don't tell me :) it needs to be in the documentation!

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Nov 14, 2014

Contributor

ALSO. is there a limitation on the set of Dockerfile instructions that can be done via --change? because there seems to be nothing documenting which instructions can work.

Contributor

SvenDowideit commented Nov 14, 2014

ALSO. is there a limitation on the set of Dockerfile instructions that can be done via --change? because there seems to be nothing documenting which instructions can work.

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Nov 14, 2014

Contributor

I can't see any tests showing if 2 new layers gets made (which imo would be the expectation most users would have), or there's only one layer, and then that different combinations of --change params get applied as expected.

I also don't see tests that make clear which instructions do and don't get applied - I'm assuming from the references to image JSON (which is new in the docs and would need more info too) that i can't --change RUN apt-get update && apt-get install -yq yum - but you havn't said.

Contributor

SvenDowideit commented Nov 14, 2014

I can't see any tests showing if 2 new layers gets made (which imo would be the expectation most users would have), or there's only one layer, and then that different combinations of --change params get applied as expected.

I also don't see tests that make clear which instructions do and don't get applied - I'm assuming from the references to image JSON (which is new in the docs and would need more info too) that i can't --change RUN apt-get update && apt-get install -yq yum - but you havn't said.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Nov 17, 2014

Contributor
  • Supported Dockerfile instructions: CMD, ENTRYPOINT, ENV, EXPOSE, ONBUILD, USER, VOLUME, WORKDIR

RUN/ADD are not supported.

When this patch gets applied, I plan on working on another patch to allow MAINTAINER to be applied. But I want this to get in first.

Contributor

rhatdan commented Nov 17, 2014

  • Supported Dockerfile instructions: CMD, ENTRYPOINT, ENV, EXPOSE, ONBUILD, USER, VOLUME, WORKDIR

RUN/ADD are not supported.

When this patch gets applied, I plan on working on another patch to allow MAINTAINER to be applied. But I want this to get in first.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Nov 17, 2014

Contributor

@SvenDowideit Please give me the wording you would like for the describing the one layer that gets created.

Contributor

rhatdan commented Nov 17, 2014

@SvenDowideit Please give me the wording you would like for the describing the one layer that gets created.

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Nov 18, 2014

Contributor

@rhatdan please see rhatdan#1

and once that's merged - LGTM - @fredlf @jamtur01

Contributor

SvenDowideit commented Nov 18, 2014

@rhatdan please see rhatdan#1

and once that's merged - LGTM - @fredlf @jamtur01

@fredlf

This comment has been minimized.

Show comment
Hide comment
@fredlf

fredlf Nov 27, 2014

Contributor

Docs LGTM

Contributor

fredlf commented Nov 27, 2014

Docs LGTM

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Dec 3, 2014

Contributor

@crosbymichael I've assigned you to this because you indicated you would look into something for it 11 days ago.

Contributor

SvenDowideit commented Dec 3, 2014

@crosbymichael I've assigned you to this because you indicated you would look into something for it 11 days ago.

@icecrime icecrime removed this from the 1.5.0 milestone Jan 19, 2015

@crosbymichael crosbymichael added this to the 1.6.0 milestone Feb 6, 2015

Show outdated Hide outdated api/client/commands.go
@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Feb 9, 2015

Contributor

@crosbymichael @shykes @tianon Can we get this reviewed and in finally in... Last complaint, I have heard was around the \n.

Contributor

rhatdan commented Feb 9, 2015

@crosbymichael @shykes @tianon Can we get this reviewed and in finally in... Last complaint, I have heard was around the \n.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Feb 13, 2015

Contributor

Any update?

Contributor

rhatdan commented Feb 13, 2015

Any update?

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Feb 13, 2015

Collaborator

@rhatdan I have this on my todo list, I'll review it today. I really want this too :)

Collaborator

tiborvass commented Feb 13, 2015

@rhatdan I have this on my todo list, I'll review it today. I really want this too :)

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Feb 17, 2015

Contributor

This has the update to pass the changes via the url params via

for _, change := range flChanges.GetAll() {
        v.Add("changes", change)
}

This should be good to merged now if you can take another look.

@tiborvass @LK4D4

Contributor

crosbymichael commented Feb 17, 2015

This has the update to pass the changes via the url params via

for _, change := range flChanges.GetAll() {
        v.Add("changes", change)
}

This should be good to merged now if you can take another look.

@tiborvass @LK4D4

@crosbymichael crosbymichael removed their assignment Feb 20, 2015

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Feb 23, 2015

Contributor

LGTM

Contributor

crosbymichael commented Feb 23, 2015

LGTM

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Feb 23, 2015

Contributor

Two more LGTM to make my day...
@tiborvass @LK4D4

Contributor

rhatdan commented Feb 23, 2015

Two more LGTM to make my day...
@tiborvass @LK4D4

Show outdated Hide outdated api/server/server.go
img, err := s.graph.Create(archive, "", "", "Imported from "+src, "", nil, nil)
buildConfigJob := job.Eng.Job("build_config")
buildConfigJob.Stdout.Add(stdoutBuffer)

This comment has been minimized.

@tiborvass

tiborvass Feb 23, 2015

Collaborator

@rhatdan Could we do a r, w := io.Pipe() and send the writer to stdout, and call json.NewDecoder(r) to avoid additional buffers?

EDIT: that would be blocking

@tiborvass

tiborvass Feb 23, 2015

Collaborator

@rhatdan Could we do a r, w := io.Pipe() and send the writer to stdout, and call json.NewDecoder(r) to avoid additional buffers?

EDIT: that would be blocking

This comment has been minimized.

@rhatdan

rhatdan Feb 24, 2015

Contributor

Well since I had no idea how to do this, I guess I don't have to. :^)

@rhatdan

rhatdan Feb 24, 2015

Contributor

Well since I had no idea how to do this, I guess I don't have to. :^)

dqminh and others added some commits Sep 29, 2014

pass --change changes to the commit job
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: rhatdan)
support `changes` in commit job
In addition to config env, `commit` now will also accepts a `changes` env which
is a string contains new-line separated Dockerfile instructions.
`commit` will evaluate `changes` into `runconfig.Config` and merge it with
`config` env, and then finally commit a new image with the changed config

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: rhatdan)
instantiate the builder job in commit integration tests
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: rhatdan)
Return error on invalid --change command
Signed-off-by: Michael Crosby <crosbymichael@gmail.com>

Docker-DCO-1.1-Signed-off-by: Michael Crosby <crosbymichael@gmail.com> (github: rhatdan)
build_config job: parse dockerfile ast into config
Instead of building the actual image, `build_config` will serialize a subset of
dockerfile ast into *runconfig.Config

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)
add docs for commit --change
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)
I am only seeing the values I set
Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)
pass --change changes to the import job
Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)
@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Feb 24, 2015

Collaborator

LGTM

Collaborator

tiborvass commented Feb 24, 2015

LGTM

tiborvass added a commit that referenced this pull request Feb 24, 2015

Merge pull request #9123 from rhatdan/commit-change
Patch to commit-change patch to add docker import support

@tiborvass tiborvass merged commit e7dc7a6 into moby:master Feb 24, 2015

1 check passed

janky Jenkins build Docker-PRs 1750 has succeeded
Details
@ahmetb

This comment has been minimized.

Show comment
Hide comment
@ahmetb

ahmetb Feb 25, 2015

Contributor

Hey folks this breaks if the busybox image in the test environment is not built from jpettazzo's busybox (https://github.com/docker/docker/blob/309eec23787d9e1e4de58b6475467a1af73c4c26/Dockerfile#L111) but rather pulled from docker hub (using docker pull busybox) because in the case of non-linux CLI tests we're running outside a container.

The busybox image on Docker Hub we pull in this case adds an extra PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin regardless of the --change parameter and the the test fails because inspected Config.Env has an extra PATH field. (https://github.com/docker/docker/blob/4a9fa9650b154e70d55f750c3674c2d6dd390bef/integration-cli/docker_cli_commit_test.go#L228)

How does it sound to add an extra --change ENV PATH /foo (and same in the expected string)? I tested it locally and it works fine. Is there any other way to get rid of that PATH added by busybox image on Docker Hub?

@tiborvass @dqminh @tianon

Contributor

ahmetb commented Feb 25, 2015

Hey folks this breaks if the busybox image in the test environment is not built from jpettazzo's busybox (https://github.com/docker/docker/blob/309eec23787d9e1e4de58b6475467a1af73c4c26/Dockerfile#L111) but rather pulled from docker hub (using docker pull busybox) because in the case of non-linux CLI tests we're running outside a container.

The busybox image on Docker Hub we pull in this case adds an extra PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin regardless of the --change parameter and the the test fails because inspected Config.Env has an extra PATH field. (https://github.com/docker/docker/blob/4a9fa9650b154e70d55f750c3674c2d6dd390bef/integration-cli/docker_cli_commit_test.go#L228)

How does it sound to add an extra --change ENV PATH /foo (and same in the expected string)? I tested it locally and it works fine. Is there any other way to get rid of that PATH added by busybox image on Docker Hub?

@tiborvass @dqminh @tianon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment