-
Notifications
You must be signed in to change notification settings - Fork 2k
Add support for multiple platform options in image load and save #6126
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6126 +/- ##
==========================================
+ Coverage 55.02% 55.05% +0.03%
==========================================
Files 361 361
Lines 30152 30161 +9
==========================================
+ Hits 16591 16605 +14
+ Misses 12604 12598 -6
- Partials 957 958 +1 🚀 New features to boost your workflow:
|
Signed-off-by: Cesar Talledo <[email protected]>
Signed-off-by: Cesar Talledo <[email protected]>
54b967e
to
8993f54
Compare
…/save. Signed-off-by: Cesar Talledo <[email protected]>
name: "with-comma-separated-platforms", | ||
args: []string{"--platform", "linux/amd64,linux/arm64/v8,linux/riscv64"}, | ||
imageLoadFunc: func(input io.Reader, options ...client.ImageLoadOption) (image.LoadResponse, error) { | ||
assert.Check(t, len(options) > 0) // can be 1 or two depending on whether a terminal is attached :/ |
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.
Hmm I'm not sure I understand this one. How does terminal affect parsing here?
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 was simply copying a comment in the test right above this line (i.e., "with-single-platform"
), but the underlying rationale for the comment comes from this code:
func runLoad(ctx context.Context, dockerCli command.Cli, opts loadOptions) error {
...
var options []client.ImageLoadOption
if opts.quiet || !dockerCli.Out().IsTerminal() {
options = append(options, client.ImageLoadWithQuiet(true))
}
...
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.
Ah right.. We should apply these opts to a temporary var and then check the resulting opts struct, but we can't do that because these types are unexported..
I guess the only way to test it in this situation is by checking HTTP request at the HTTP client, but that would need rewriting the test a bit.
Although, I think the best solution would be to have a debug helper on the client side that would allow introspecting for testing purposes...
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.
We should do that in a follow up though
FWIW; I had a "wip" branch to implement a multi-platform option, but it still needs some work (and a decision where we want to put it); |
Changed the changelog a bit, let me know if that looks good. cc @ArthurFlag |
Thanks @vvoland, LGTM. |
- What I did
Prior to this change,
docker image load
anddocker image save
accept only a single platform via the--platform
option.This change adds support for multiple platforms using a comma-separated list passed to
--platform
. E.g.:** NOTE **: Depends on the corresponding change in the Moby engine (see moby/moby#50166).
- How I did it
Updated the
--platform
option in theimage load
andimage save
commands to accept an array of platforms, as opposed to a single platform.- How to verify it
Run updated unit tests.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)