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

docker commit should not drop configuration #1141

Closed
mhennings opened this issue Jul 5, 2013 · 22 comments · Fixed by #4000
Closed

docker commit should not drop configuration #1141

mhennings opened this issue Jul 5, 2013 · 22 comments · Fixed by #4000

Comments

@mhennings
Copy link
Contributor

@mhennings mhennings commented Jul 5, 2013

Currently when doing a commit on a container to update an image, the configuration the image had is not kept.

I think there should be an option to drop the previous config and a default to keep it.

@fkautz

This comment has been minimized.

Copy link
Contributor

@fkautz fkautz commented Aug 16, 2013

What configuration are you thinking of?

E.g. dns? mounted volumes?

@mhennings

This comment has been minimized.

Copy link
Contributor Author

@mhennings mhennings commented Aug 16, 2013

i had volumes and the cmd in mind.

@asbjornenge

This comment has been minimized.

Copy link
Contributor

@asbjornenge asbjornenge commented Sep 13, 2013

👍 this hit me hard today :-/ Built up quite a huge Dockerfile with lots of ports EXPOSEd. If I do some work in the container and want to commit as a new image, all ports and also CMD is lost.

@jpetazzo

This comment has been minimized.

Copy link
Contributor

@jpetazzo jpetazzo commented Sep 13, 2013

+1

On Fri, Sep 13, 2013 at 9:55 AM, asbjornenge notifications@github.comwrote:

[image: 👍] this hit me hard today :-/ Built up quite a huge Dockerfile
with lots of ports EXPOSEd. If I do some work in the container and want to
commit as a new image, all ports and also CMD is lost.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1141#issuecomment-24408559
.

@jpetazzo https://twitter.com/jpetazzo
Latest blog post: http://blog.docker.io/2013/09/docker-joyent-openvpn-bliss/

@tylrtrmbl

This comment has been minimized.

Copy link

@tylrtrmbl tylrtrmbl commented Sep 14, 2013

This was a big one for me today as well!

@asbjornenge

This comment has been minimized.

Copy link
Contributor

@asbjornenge asbjornenge commented Sep 18, 2013

A friendly tip (might be obvious to most):

As a workaround I just keep a separate Dockerfile with only the configuration stuff that is lost, and the image I'm committing to as FROM. It's not awesome, but it works.

@tylrtrmbl

This comment has been minimized.

Copy link

@tylrtrmbl tylrtrmbl commented Sep 18, 2013

Haha! A dirty hack, but I will use it 'til we see this closed!

Thanks. :)

@vampolo

This comment has been minimized.

Copy link

@vampolo vampolo commented Sep 30, 2013

+1

@icook

This comment has been minimized.

Copy link

@icook icook commented Oct 12, 2013

+1

1 similar comment
@athieriot

This comment has been minimized.

Copy link

@athieriot athieriot commented Oct 18, 2013

+1

@BMorearty

This comment has been minimized.

Copy link

@BMorearty BMorearty commented Oct 31, 2013

+1

In addition, docker history imagename should show the configuration overrides that were passed via the -run option to docker commit. So you can see the run configuration.

I know you can do docker inspect imagename but that's all the configuration, not just the overrides from the command line. It's hard to see what the deltas are.

@creack

This comment has been minimized.

Copy link
Contributor

@creack creack commented Nov 6, 2013

ping @shykes, any thoughts?

@parente

This comment has been minimized.

Copy link

@parente parente commented Dec 4, 2013

+1 on preserving cmd, ports, env, etc.

@shykes

This comment has been minimized.

Copy link
Contributor

@shykes shykes commented Jan 6, 2014

I believe this has been fixed, @creack @crosbymichael @vieux can you confirm?

@crosbymichael

This comment has been minimized.

Copy link
Member

@crosbymichael crosbymichael commented Jan 6, 2014

No this is still and issue with docker commit. It was been fixed with docker build but still an issue with commit.

Tagging as easy fix

@cap10morgan

This comment has been minimized.

Copy link
Contributor

@cap10morgan cap10morgan commented Jan 29, 2014

Any progress on this? Got bit by this today on 0.7.6.

cap10morgan added a commit to cap10morgan/docker that referenced this issue Jan 31, 2014
@cap10morgan

This comment has been minimized.

Copy link
Contributor

@cap10morgan cap10morgan commented Jan 31, 2014

I'm working on this issue, and it is indeed mostly an easy fix. But the wrinkle I've run into is figuring out when the user has specified a new config via commit's -run option. Hopefully there's an easy solution for someone who's been using Go for more than a few minutes.

Here's my progress so far: https://github.com/cap10morgan/docker/compare/fix-issue-1141

The if false is where I need to figure out a way to tell if the user submitted a new config (and then do we merge or replace?).

I've tried comparing the Config struct to an empty one (Config{}), but that doesn't work because there are slice members and they don't support equality comparison.

I've also tried checking for no incoming config JSON, but something in the command -> API -> server cycle is putting an empty struct's JSON representation in there anyway, so that's stuck at the same place.

So does anyone have any tips for a robust way of detecting when the user has submitted a new config on the commit command line?

OR

Should I just work on a shallow merge function that leaves the original container's config values there when there is no value in the request's config but otherwise clobbers it with the request's config values? This would be more work, I think, but if it's the right thing to do (and it seems like it is), then it would obviate the need to detect whether someone had given any config values via the -run option (since they'll all be empty and thus won't override the original container's config).

@cap10morgan

This comment has been minimized.

Copy link
Contributor

@cap10morgan cap10morgan commented Feb 1, 2014

Update: I just found CompareConfig and MergeConfig in utils.go. I'll just use MergeConfig.

cap10morgan added a commit to cap10morgan/docker that referenced this issue Feb 1, 2014
@cap10morgan

This comment has been minimized.

Copy link
Contributor

@cap10morgan cap10morgan commented Feb 1, 2014

Pull request here: #3889

@prologic

This comment has been minimized.

Copy link
Contributor

@prologic prologic commented Feb 1, 2014

+1

cap10morgan added a commit to cap10morgan/docker that referenced this issue Feb 3, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
@cap10morgan

This comment has been minimized.

Copy link
Contributor

@cap10morgan cap10morgan commented Feb 3, 2014

I closed that PR so I could submit a new one that follows the Docker contrib guidelines a bit better and adds a test. However, I'm a bit stuck on the best way to test that the merged config is correct: cap10morgan@2cf4727

Any tips would be most welcome.

cap10morgan added a commit to cap10morgan/docker that referenced this issue Feb 7, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)

WIP adding a test for merging configs on commit

I need some advice here, see comment at the end of the diff.

remove unused srv from merge commit integration test

use config1 instead of config in merge commit integration test

WIP testing merge-configs-on-commit

finish merge-configs-on-commit integration test
cap10morgan added a commit to cap10morgan/docker that referenced this issue Feb 10, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
cap10morgan added a commit to cap10morgan/docker that referenced this issue Feb 10, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
cap10morgan added a commit to cap10morgan/docker that referenced this issue Feb 15, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
cap10morgan added a commit to cap10morgan/docker that referenced this issue Feb 27, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
cap10morgan added a commit to cap10morgan/docker that referenced this issue Feb 28, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
@mattva01

This comment has been minimized.

Copy link

@mattva01 mattva01 commented Mar 9, 2014

+1

@creack creack closed this in #4000 Mar 13, 2014
kippandrew added a commit to kippandrew/docker that referenced this issue Mar 17, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
unclejack added a commit to unclejack/moby that referenced this issue Mar 18, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
@pda pda mentioned this issue Mar 27, 2014
1 of 3 tasks complete
kenshin54 added a commit to dianping/docker that referenced this issue Apr 9, 2014
Fixes moby#1141

Docker-DCO-1.1-Signed-off-by: Wes Morgan <cap10morgan@gmail.com> (github: cap10morgan)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.