Uh oh!
There was an error while loading. Please reload this page.
[18.09] Refine how metadata dir is handled - #1388
Conversation
Codecov Report
@@ Coverage Diff @@## 18.09 #1388 +/- ##
==========================================
- Coverage 54.15% 54.09% -0.06%
==========================================
Files 290 290 Lines 19298 19313 +15 ==========================================
- Hits 10451 10448 -3 - Misses 8181 8201 +20 + Partials 666 664 -2 |
dhiltgen
commented
Sep 28, 2018
Note: this depends on the packaging change PR(s) |
thaJeztah
left a comment
There was a problem hiding this comment.
Did a quick read over the code, left some comments
| authConfig *types.AuthConfig, healthfn func(context.Context) error) error { | ||
| authConfig *types.AuthConfig, healthfn func(context.Context) error, runtimeMetadataDir string) error { | ||
| // If the user didn't specify an image, detemrine the correct enterprise image to use |
There was a problem hiding this comment.
nit: typo; s/detemrine/determine/
| return err | ||
| } | ||
| if strings.Contains(strings.ToLower(serverVersion.Platform.Name), "enterprise") { | ||
| return nil |
There was a problem hiding this comment.
We should either treat this as an error-situation, or inform the user that no changes were made. In the "happy path", a message will be printed "Successfully activated engine". If we're already on EE, I'd expect some form of feedback as well.
There was a problem hiding this comment.
sound good to make this change
| // ActivateEngine will switch the image from the CE to EE image | ||
| func (c *baseClient) ActivateEngine(ctx context.Context, opts clitypes.EngineInitOptions, out clitypes.OutStream, | ||
| authConfig *types.AuthConfig, healthfn func(context.Context) error) error { | ||
| authConfig *types.AuthConfig, healthfn func(context.Context) error, runtimeMetadataDir string) error { |
There was a problem hiding this comment.
Instead of adding another argument, can we add runtimeMetadataDir as a field on clitypes.EngineInitOptions ?
| out clitypes.OutStream, | ||
| authConfig *types.AuthConfig, | ||
| healthfn func(context.Context) error) error { | ||
| healthfn func(context.Context) error, runtimeMetadataDir string) error { |
There was a problem hiding this comment.
Same here; Instead of adding another argument, can we add runtimeMetadataDir as a field on clitypes.EngineInitOptions ?
There was a problem hiding this comment.
Wait: what's that healthfn argument actually for? Looks like it's only used for testing? That really shouldn't be in there; I'm sure that can be set on the mock during testing?
There was a problem hiding this comment.
Hmm.. hold over from prior implementations... we really need to get this in ASAP so let me excise that part in a follow up if you don't mind.
| _, err := client.Ping(ctx) | ||
| return err | ||
| }); err != nil { | ||
| }, clitypes.RuntimeMetadataDir); err != nil { |
There was a problem hiding this comment.
Related to my other comments about adding this option to the clitypes.EngineInitOptions; it looks like currently we only pass the default value.
We could get that default in client.ActivateEngine() / client.DoUpdate(), i.e., something like
func (c*baseClient) DoUpdate(ctx context.Context, opts clitypes.EngineInitOptions, out clitypes.OutStream,
authConfig*types.AuthConfig, healthfnfunc(context.Context) error) error {
ifopts.MetaDataDir=="" {
opts.MetaDataDir=clitypes.RuntimeMetadataDir
}
...
}Doing so, makes the default (empty string) a useful value
thaJeztah
commented
Sep 28, 2018
ping @silvin-lubecki@vdemeester PTAL |
dhiltgen
commented
Sep 28, 2018
@thaJeztah if you're still online, could you take a quick peek? We'd like to get this in today if we can so we can cut a beta1.8 build to wrap up a number of remaining P0's. |
thaJeztah
left a comment
There was a problem hiding this comment.
changes SGTM, but could use some more eyes on this one
please squash the commits
crosbymichael
commented
Sep 28, 2018
LGTM |
This is a follow up PR to docker#1381 to address some of the review comments we didn't get to. Signed-off-by: Daniel Hiltgen <daniel.hiltgen@docker.com>
This is a follow up PR to #1381 to address some of the review comments
we didn't get to.
Signed-off-by: Daniel Hiltgen daniel.hiltgen@docker.com