Skip to content

Add support for 'commit --change'#8332

Closed
dqminh wants to merge 4 commits intomoby:masterfrom
dqminh:docker-commit-change
Closed

Add support for 'commit --change'#8332
dqminh wants to merge 4 commits intomoby:masterfrom
dqminh:docker-commit-change

Conversation

@dqminh
Copy link
Copy Markdown
Contributor

@dqminh dqminh commented Oct 1, 2014

The spec for commit --change can be found at #5105
This PR changes the original pull request to use new builder for generating runconfig.Config

Supported Dockerfile instructions for --change are:

- ENTRYPOINT
- CMD
- USER
- WORKDIR
- ENV
- VOLUME
- EXPOSE
- ONBUILD

Example command:

> docker run --name test busybox true
> docker commit --change "EXPOSE 8080 3000" --change "ENV DEBUG true" --change "VOLUME /test1 /test2" --change "WORKDIR /test1" test test-commit
> docker inspect test-commit
"Config": {
  "Env": [
    "DEBUG=true",
    "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
  ],
  "ExposedPorts": {
    "3000/tcp": {},
    "8080/tcp": {}
  },
  "Volumes": {
    "/test1": {},
    "/test2": {}
  },
  "WorkingDir": "/test1"
},

@rhatdan
Copy link
Copy Markdown
Contributor

rhatdan commented Oct 16, 2014

This looks perfect to me, although I would like to see the same changes applied to docker import. Adding this to import would allow people building base images to properly annotate them.

@crosbymichael @shykes @tianon Can we get thumbs up or thumbs down on this pull request. It does match the previous suggested syntax and allows us a way forward with some of my other pull requests and allows me to resubmit the META patch.

@dqminh
Copy link
Copy Markdown
Contributor Author

dqminh commented Oct 20, 2014

@rhatdan yes, i would like to see --change in docker import too. I think that once the design for this patch is approved, it will not be hard to create a patch for import that supports the same functionalities.

@rhatdan rhatdan mentioned this pull request Oct 20, 2014
Closed
@rhatdan
Copy link
Copy Markdown
Contributor

rhatdan commented Oct 20, 2014

@dqminh Could you rebase this patch set to allow it to merge.

dqminh added 4 commits October 20, 2014 21:31
@dqminh
pass --change changes to the commit job
e88fc7d 
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <[email protected]> (github: dqminh)
@dqminh
build_config job: parse dockerfile ast into config
649c558 
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 <[email protected]> (github: dqminh)
@dqminh
support changes in commit job
6a24c17 
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 <[email protected]> (github: dqminh)
@dqminh
instantiate the builder job in commit integration tests
e2a4052 
Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <[email protected]> (github: dqminh)
@dqminh dqminh force-pushed the docker-commit-change branch from b78e3c6 to e2a4052 Compare October 21, 2014 01:33
@dqminh
Copy link
Copy Markdown
Contributor Author

dqminh commented Oct 21, 2014

@rhatdan I rebased the patch. I think this will need docs too, i will do it once the design is approved.

@rhatdan
Copy link
Copy Markdown
Contributor

rhatdan commented Oct 21, 2014

@crosbymichael Does this look like what you guys want? If we are going to close down my pull request for setting environment variables in docker import, I need a way forward.

@rhatdan
Copy link
Copy Markdown
Contributor

rhatdan commented Oct 24, 2014

Acceptance of this patch is holding up at least two of our patches, from even being submitted?

Can we get approval of the design?

@crosbymichael @tianon @shykes @creack

Someone from docker?????

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Oct 24, 2014

Reviewing

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Oct 24, 2014

For things that are not supported I think the user should get a hard error. I tried this and it worked.

docker commit --change 'FROM other' 7334db741b97
bb501d8c9d8257c2a703a718c8f1a25bb2a1ecb847dc2997bfb114aa906d58ea

@rhatdan
Copy link
Copy Markdown
Contributor

rhatdan commented Oct 24, 2014

I agree, FROM, RUN, ADD should all be blocked. But if the CLI is fine, we could move forward.
I just want ENV, CMD, MAINTAINER and META when this patch gets in.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 24, 2014

I agree, FROM, RUN, ADD should all be blocked

Perhaps an implementation detail, but probably better to whitelist accepted instructions in stead of blacklist, e.g. COPY should be blocked as well. Whitelisting is easier to maintain if future commands are added.

@crosbymichael crosbymichael mentioned this pull request Oct 24, 2014
Closed
@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Oct 24, 2014

I opened a pr for the whitelist and return of the error.

@rhatdan
Copy link
Copy Markdown
Contributor

rhatdan commented Oct 25, 2014

Link?

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 25, 2014

@rhatdan here: #8765

@gregorskii gregorskii mentioned this pull request Jan 23, 2017
Merged
@thaJeztah thaJeztah mentioned this pull request Jun 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dqminh @rhatdan @crosbymichael @thaJeztah