-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Conversation
docs LGTM - though I'd love an example :) |
@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. :) |
@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 |
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. |
There was a problem hiding this comment.
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)
I made your suggested changes, @SvenDowideit. |
LGTM Seems to be working well so far. There is a separate issue that the CMD is empty and is being committed as |
Docs LGTM, thank you very much @cap10morgan |
No problem, it was fun. :) Do you all need anything else from me to get this merged? What's the process from here? |
@cap10morgan This needs rebased because the files were moved. |
@crosbymichael Rebased. |
LGTM, but we should wait 0.8.1 to merge |
ping @creack wanna take a look at this before merge? |
We will wait on this one for a day until @creack can take a look |
(and @shykes) |
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. |
@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. |
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. |
ping @unclejack |
ping @cap10morgan Can you rebase, please? |
@unclejack rebased |
@cap10morgan The tests fail:
Building Docker fails for the same reason. |
@unclejack Really? But the Travis CI build is green. |
@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. |
@cap10morgan You can find the results of the tests for PRs here: http://docker-ci.dotcloud.com/waterfall |
@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)
@unclejack I think it's fixed now. |
ping @creack |
1 similar comment
ping @creack |
LGTM |
merge existing config when committing
Any chance to see this out soon? |
For future reference: this was deprecated and removed from the documentation less than a month later: 168f8ab |
@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. |
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.