-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
add dry-run support to create command #10413
Conversation
return moby.ImageInspect{ID: "dryRunId"}, nil, nil | ||
caller := getCallingFunction() | ||
switch caller { | ||
case "pullServiceImage", "buildContainerVolumes": |
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.
looks like a fragile solution to distinguish usages, but I guess we don't have a better option :'(
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.
True, but as we can't change the interface signature and we don't want to apply the same behaviour depending of the parent context, this was the first solution in my mind.
I can try to use a wrapper function to call ImageInspectWithRaw
instead, wdyt?
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.
I wonder: would it make sense we pass the top-level command being ran as a Context.Value
so it make it easier to distinguish usages?
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.
It's not enough for example if you run the create
command, the ImageInspectWithRaw
function will be call multiple times, first to check if the image is present locally, so we want to get an error if the image doesn't exists to trigger a pull and after the pull (and this time we don't want to fail because we just faked the pull)
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.
nice... which seems to demonstrate some design issue to be fixed, as we should not require a second call after pull :P
anyway, go for it
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.
I think the second call is here to very the pull was correctly done, kind of double check protection in some way
Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
6d016d8
to
b83edbd
Compare
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## v2 #10413 +/- ##
==========================================
- Coverage 60.05% 59.84% -0.21%
==========================================
Files 104 104
Lines 9029 9048 +19
==========================================
- Hits 5422 5415 -7
- Misses 3038 3062 +24
- Partials 569 571 +2
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
What I did
Add support of dry-run mode to
create
commandRelated issue
https://docker.atlassian.net/browse/ENV-52
(not mandatory) A picture of a cute animal, if possible in relation to what you did