Extract daemon configuration and discovery to their own package by vdemeester · Pull Request #29673 · moby/moby

vdemeester · 2016-12-23T08:56:06Z

The daemon package is really big and not well unit tested. This small step extract 2 relatively less coupled functionnality of daemon into their own package.

This also moves some cli specific in cmd/dockerd as it does not really belong to the daemon/config package. There is still a pflags dependency on the valueSet field, I added a comment on that to fix.

This is the coverage almost as is — just complete the Validate test, but the diff is getting pretty big already so I'll do a follow-up for the tests.

ok      github.com/docker/docker/daemon/config  0.010s  coverage: 60.2% of statements
ok      github.com/docker/docker/daemon/discovery       0.006s  coverage: 31.9% of statements

/cc @dnephin @thaJeztah @icecrime @cpuguy83 @crosbymichael @AkihiroSuda @tonistiigi

🐸

Signed-off-by: Vincent Demeester [email protected]

dnephin

Seems like a good incremental step. Still a lot of work to do to make daemon a sane package.

dnephin · 2016-12-27T17:49:25Z

cmd/dockerd/config_common_unix.go

CommonUnix seems like a strange name here. Maybe installUnixConfigFlags() ?

same goes for the other CommonUnix variables

dnephin · 2016-12-27T17:51:04Z

daemon/config/config.go

This constant appears to be only used in one place. It would be better to keep it unexported and move it to where it is used.

dnephin · 2016-12-27T17:59:50Z

daemon/config/config_solaris.go

Is this still called? It seems like it was removed from the other platforms, so I think this might be broken on solaris.

vdemeester · 2016-12-28T08:59:07Z

@dnephin updated 👼

Still a lot of work to do to make daemon a sane package.

You don't say 😅

thaJeztah · 2016-12-28T15:44:25Z

needs a rebase now @vdemeester 😢

vdemeester · 2016-12-28T17:45:04Z

Hum windows failure is legit.. messed up build tags 😅

thaJeztah

LGTM if green

vdemeester · 2017-01-09T13:45:02Z

Rebased 👼

cpuguy83 · 2017-01-09T14:07:02Z

Build errors

vdemeester · 2017-01-09T14:09:57Z

arf… damn rebase 😂

dnephin · 2017-02-01T16:07:35Z

needs a rebase

thaJeztah · 2017-02-01T18:33:28Z

@vdemeester test is failing on windows;

7:56:09 # github.com/docker/docker/cmd/dockerd
17:56:09 cmd\dockerd\config_unix_test.go:22: conf.ShmSize undefined (type *"github.com/docker/docker/daemon/config".Config has no field or method ShmSize)
17:56:09 cmd\dockerd\config_unix_test.go:23: conf.ShmSize undefined (type *"github.com/docker/docker/daemon/config".Config has no field or method ShmSize)
17:56:09 cmd\dockerd\config_unix_test.go:27: conf.ShmSize undefined (type *"github.com/docker/docker/daemon/config".Config has no field or method ShmSize)
17:56:09 cmd\dockerd\config_unix_test.go:28: conf.ShmSize undefined (type *"github.com/docker/docker/daemon/config".Config has no field or method ShmSize)
17:56:09 FAIL	github.com/docker/docker/cmd/dockerd [build failed]

vdemeester · 2017-02-07T11:38:49Z

/ping @justincormack @runcom @AkihiroSuda 😝

allencloud · 2017-02-08T02:32:40Z

need rebase again ^^ @vdemeester

This also moves some cli specific in `cmd/dockerd` as it does not really belong to the `daemon/config` package. Signed-off-by: Vincent Demeester <[email protected]>

LK4D4

LGTM

GordonTheTurtle added the status/0-triage label Dec 23, 2016

vdemeester added status/2-code-review and removed status/0-triage labels Dec 23, 2016

vdemeester force-pushed the extract-daemon-config branch 4 times, most recently from 8ede593 to 58713b2 Compare December 25, 2016 19:26

vdemeester added the rebuild/janky label Dec 25, 2016

GordonTheTurtle removed the rebuild/janky label Dec 25, 2016

dnephin reviewed Dec 27, 2016

View reviewed changes

vdemeester force-pushed the extract-daemon-config branch from 58713b2 to 02302fb Compare December 28, 2016 08:58

vdemeester force-pushed the extract-daemon-config branch from 02302fb to 719ead9 Compare December 28, 2016 15:59

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 28, 2016

vdemeester force-pushed the extract-daemon-config branch from 719ead9 to 6debd1c Compare December 28, 2016 17:52

vdemeester removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 28, 2016

thaJeztah approved these changes Dec 28, 2016

View reviewed changes

vdemeester added the rebuild/windowsRS1 label Dec 29, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Dec 29, 2016

vdemeester force-pushed the extract-daemon-config branch from 6debd1c to 0f5fae8 Compare December 30, 2016 17:39

vdemeester force-pushed the extract-daemon-config branch from 0f5fae8 to 336992c Compare January 9, 2017 13:44

vdemeester force-pushed the extract-daemon-config branch 3 times, most recently from 58d9bd8 to a9ebdb6 Compare January 12, 2017 15:54

vdemeester force-pushed the extract-daemon-config branch from a9ebdb6 to 09d5edd Compare January 23, 2017 11:23

vdemeester force-pushed the extract-daemon-config branch from 09d5edd to ad492fa Compare January 30, 2017 13:25

vdemeester force-pushed the extract-daemon-config branch 6 times, most recently from 27fbc52 to f01a03c Compare February 1, 2017 17:47

vdemeester force-pushed the extract-daemon-config branch from f01a03c to 0c9630a Compare February 2, 2017 09:55

vdemeester added the rebuild/windowsRS1 label Feb 2, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Feb 2, 2017

vdemeester mentioned this pull request Feb 7, 2017

refactor config reload in daemon #30303

Merged

Extract daemon configuration and discovery to their own package

db63f93

This also moves some cli specific in `cmd/dockerd` as it does not really belong to the `daemon/config` package. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the extract-daemon-config branch from 0c9630a to db63f93 Compare February 8, 2017 08:54

GordonTheTurtle assigned LK4D4 Feb 10, 2017

LK4D4 approved these changes Feb 13, 2017

View reviewed changes

LK4D4 merged commit f3a8886 into moby:master Feb 13, 2017

GordonTheTurtle added this to the 1.14.0 milestone Feb 13, 2017

vdemeester deleted the extract-daemon-config branch February 13, 2017 17:51

thaJeztah mentioned this pull request Feb 14, 2017

Deprecate --graph flag; Replace with --data-root #28696

Merged

thaJeztah mentioned this pull request Apr 3, 2017

Init Binary no longer set in output of docker info #32314

Closed

Conversation

vdemeester commented Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Dec 28, 2016

Uh oh!

thaJeztah commented Dec 28, 2016

Uh oh!

vdemeester commented Dec 28, 2016

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Jan 9, 2017

Uh oh!

cpuguy83 commented Jan 9, 2017

Uh oh!

vdemeester commented Jan 9, 2017

Uh oh!

dnephin commented Feb 1, 2017

Uh oh!

thaJeztah commented Feb 1, 2017

Uh oh!

vdemeester commented Feb 7, 2017

Uh oh!

allencloud commented Feb 8, 2017

Uh oh!

LK4D4 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vdemeester commented Dec 23, 2016 •

edited

Loading