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

merge existing config when committing #4000

Merged
merged 1 commit into from
Mar 13, 2014

Conversation

cap10morgan
Copy link
Contributor

Fixes #1141 by merging parent config with any -run supplied config when committing containers to images. If no -run config is given, the committed image inherits the parent's entire config. Any keys given with -run will overwrite the corresponding keys' values from the parent.

@SvenDowideit
Copy link
Contributor

docs LGTM - though I'd love an example :)

@cap10morgan
Copy link
Contributor Author

@SvenDowideit I'm not sure what would be the most instructive thing to show in an example.

To me, this change takes Docker from "wildly surprising and disappointing" to "totally expected and taken for granted that it would work this way" when it comes to committing. So I'm not sure how an example enhances that. But if you have something in mind, I'm happy to write it up and push another commit. :)

@SvenDowideit
Copy link
Contributor

@cap10morgan y, making useful examples is hard, I'd use one here to explicitly confirm the user's expectations, and as a signal to older users that things have changed

@cap10morgan
Copy link
Contributor Author

I added an example, fell on my face a bit with ReStructuredText, and force-pushed a new squashed commit to bring back the DCO.

CMD ["/usr/sbin/sshd -D"]
...

If you run that, make some changes, and then commit, Docker will merge the environment variable and exposed port configuration settings with any that you specify in the -run= option. This is a change from previous behavior where no attempt was made to preserve any existing configuration on commit.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a change from Docker 0.8.0 and before, (maybe)

@cap10morgan
Copy link
Contributor Author

I made your suggested changes, @SvenDowideit.

@crosbymichael
Copy link
Contributor

LGTM

Seems to be working well so far. There is a separate issue that the CMD is empty and is being committed as [] instead of null breaking my tests. This can be fixed separately and i'll open a PR for it.

@SvenDowideit
Copy link
Contributor

Docs LGTM, thank you very much @cap10morgan

@cap10morgan
Copy link
Contributor Author

No problem, it was fun. :)

Do you all need anything else from me to get this merged? What's the process from here?

@crosbymichael
Copy link
Contributor

@cap10morgan This needs rebased because the files were moved.

@cap10morgan
Copy link
Contributor Author

@crosbymichael Rebased.

@crosbymichael
Copy link
Contributor

ping @vieux @creack

@vieux
Copy link
Contributor

vieux commented Feb 17, 2014

LGTM, but we should wait 0.8.1 to merge

@crosbymichael
Copy link
Contributor

ping @creack

wanna take a look at this before merge?

@crosbymichael
Copy link
Contributor

We will wait on this one for a day until @creack can take a look

@creack
Copy link
Contributor

creack commented Feb 19, 2014

(and @shykes)

@creack
Copy link
Contributor

creack commented Feb 19, 2014

We made commit drop the config for a reason. I need to check the use cases and see if there are impossible scenario with this PR.

@cap10morgan
Copy link
Contributor Author

@creack I would love to know what the original reasons for dropping the config were. I asked in #docker about it before starting on the PR and everyone seemed to agree it was a bug.

I hope we can find a solution that covers whatever the original use cases were and my use case where not dropping the config is extremely useful and far less surprising behavior.

Let me know if I can help investigate.

@cap10morgan
Copy link
Contributor Author

Checking in to see if there is anything I can do to move this forward. It's slowing down our Docker-based workflows quite a bit.

@vieux
Copy link
Contributor

vieux commented Feb 21, 2014

ping @unclejack

@unclejack
Copy link
Contributor

ping @cap10morgan Can you rebase, please?

@cap10morgan
Copy link
Contributor Author

@unclejack rebased

@unclejack
Copy link
Contributor

@cap10morgan The tests fail:

++ cd .
++ go test -cover -coverprofile /go/src/github.com/dotcloud/docker/bundles/0.8.1-dev/test/coverprofiles/docker -ldflags '-X github.com/dotcloud/docker/dockerversion.GITCOMMIT "4d2d66e" -X github.com/dotcloud/docker/dockerversion.VERSION "0.8.1-dev" -w -X github.com/dotcloud/docker/dockerversion.IAMSTATIC true -linkmode external -extldflags "-lpthread -static -Wl,--unresolved-symbols=ignore-in-object-files"' -tags netgo -a
# github.com/dotcloud/docker
/tmp/go-build333881678/github.com/dotcloud/docker/_test/server.go:1365: undefined: MergeConfig
FAIL    github.com/dotcloud/docker [build failed]

Tests failed: .

Building Docker fails for the same reason.

@cap10morgan
Copy link
Contributor Author

@unclejack Really? But the Travis CI build is green.

@unclejack
Copy link
Contributor

@cap10morgan Travis checks that the code can be merged, that it's properly formatted and that the code has the proper DCO. Docker tests can't be run in the environment provided by Travis CI.

@unclejack
Copy link
Contributor

@cap10morgan You can find the results of the tests for PRs here: http://docker-ci.dotcloud.com/waterfall

@cap10morgan
Copy link
Contributor Author

@unclejack Good to know. I'll look into it today.

Fixes moby#1141

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

@unclejack I think it's fixed now.

@crosbymichael
Copy link
Contributor

ping @creack

1 similar comment
@jamtur01
Copy link
Contributor

ping @creack

@creack
Copy link
Contributor

creack commented Mar 13, 2014

LGTM

creack added a commit that referenced this pull request Mar 13, 2014
@creack creack merged commit db1afee into moby:master Mar 13, 2014
@pditommaso
Copy link

Any chance to see this out soon?

@mhsmith
Copy link

mhsmith commented Feb 13, 2015

For future reference: this was deprecated and removed from the documentation less than a month later: 168f8ab

@cap10morgan
Copy link
Contributor Author

@mhsmith Not exactly. The commit --run option was deprecated, so that no longer has any impact on this feature. But the more important use case always was and still is the inheritance of the source container's config into the image resulting from the commit.

That did not happen prior to this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker commit should not drop configuration
9 participants