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

fix daemon won't start bug caused by daemon.json and cli flags duplications #37871

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

AntaresS
Copy link
Contributor

@AntaresS AntaresS commented Sep 17, 2018

- What I did

  • bug fix for daemon refusing to start when runtimes field defined in daemon.json file as well as
    --containerd=<containerd address> --default-runtime=<name> --add-runtime=<runtime>
    passed from dockerd cli flag.
  • add skipDuplicates in daemon.go in order to allow some duplicated configurations defined in both config file and cli flag to be merged

- How I did it

- How to verify it

- Description for the changelog

Signed-off-by: Anda Xu anda.xu@docker.com

… both daemon config file and cli flags

Signed-off-by: Anda Xu <anda.xu@docker.com>
@AntaresS
Copy link
Contributor Author

cc @crosbymichael @andrewhsu

@AntaresS AntaresS changed the title fix daemon won't start bug caused by "runtimes" field conflicting with cli flag options fix daemon won't start bug caused by daemon.json and cli flags duplications Sep 17, 2018
@crosbymichael
Copy link
Contributor

LGTM

@thaJeztah
Copy link
Member

This error message was added at the time to prevent conflicting options; with this change; if both a cli-option is set, and a daemon.json is present, which configuration will be used? Does one override the other, or are configurations merged?

@AntaresS
Copy link
Contributor Author

@thaJeztah They will be merged together if both passed form cli and config file. this pr is just trying to resolve the conflict of runtimes option which is a map of slice. For other conflicts the error will still be returned.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@AntaresS would be good to have a test in a follow-up

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

Successfully merging this pull request may close these issues.

None yet

4 participants