Conversation
Codecov Report
@@ Coverage Diff @@
## master #2855 +/- ##
=========================================
Coverage ? 61.82%
=========================================
Files ? 139
Lines ? 22182
Branches ? 0
=========================================
Hits ? 13715
Misses ? 6995
Partials ? 1472 |
| m.allocator, err = allocator.New(s, m.config.PluginGetter, m.config.NetworkConfig) | ||
| // Toss all this info into a netCfg, so that the allocator can do its | ||
| // thing. | ||
| netCfg := &cnmallocator.NetworkConfig{ |
There was a problem hiding this comment.
why are you creating a new config instead of using m.config.NetworkConfig?
There was a problem hiding this comment.
^^ @dperny good point; especially in light of;
The long-pending long-awaited allocator rewrite on the new-allocator branch removes cnmallocator, and doing this refactoring in an earlier commit will simplify that operation greatly.
jtestard
left a comment
jtestard
left a comment
There was a problem hiding this comment.
Looks good, a few question to improve my understanding
|
ping @selansen ptal |
| // NetworkConfig stores network related config for the cluster | ||
| NetworkConfig *cnmallocator.NetworkConfig | ||
| // DefaultAddrPool specifies default subnet pool for global scope networks | ||
| DefaultAddrPool []string |
There was a problem hiding this comment.
Previously, a pointer was used; are there situations where these could be nil (and for a reason?)
Actually wondering; should we just copy the NetworkConfig type here, and use that instead of defining these fields both here, and in node/node.go ?
Removes inappropriate import of manager/allocator/cnmallocator into node/node.go. The consequence of this is that in upstream moby/moby, we can remove that import as well. manager/allocator/cnmallocator is an implementation detail, and importing it so high in the stack creates an unacceptably high degree of coupling. Signed-off-by: Drew Erny <drew.erny@docker.com>
dperny
force-pushed
the
remove-inappropriate-imports
branch
from
c20d5e2 to
dceb997
Compare
|
PTAL @corhere |
6 participants
Removes inappropriate import of manager/allocator/cnmallocator into node/node.go. The consequence of this is that in upstream moby/moby, we can remove that import as well. manager/allocator/cnmallocator is an implementation detail, and importing it so high in the stack creates an unacceptably high degree of coupling.
Specifically, the moby/moby code in question is here: https://github.com/moby/moby/blob/master/daemon/cluster/noderunner.go#L16
and also here:
https://github.com/moby/moby/blob/master/daemon/cluster/noderunner.go#L127-L130
The long-pending long-awaited allocator rewrite on the
new-allocatorbranch removescnmallocator, and doing this refactoring in an earlier commit will simplify that operation greatly./cc @selansen