Skip to content

Commit f594a7f

Browse files
committed
cli/command/image: remove uses of JSON field
The JSON field was added in [moby@9fd2c0f], to address [moby#19177], which reported an incompatibility with Classic (V1) Swarm, which produced a non- standard response; > Make docker load to output json when the response content type is json > Swarm hijacks the response from docker load and returns JSON rather > than plain text like the Engine does. This makes the API library to return > information to figure that out. A later change in [moby@96d7db6] added additional logic to make sure the correct content-type was returned, depending on whether the `quiet` option was set (which produced a non-JSON response). This caused inconsistency in the API response, and [moby@2f27632] changed the endpoint to always produce JSON (only skipping the "progress" output if `quiet` was set). This means that the "load" endpoint ([`imageRouter.postImagesLoad`]) now unconditionally returns JSON, making the `JSON` field fully redundant. This patch removes the use of the JSON field, as it's redundant, and the way it handles the content-type is incorrect because it would not handle correct, but different formatted response-headers (`application/json; charset=utf-8`), which could result in malformed output on the client. [moby@9fd2c0f]: moby/moby@9fd2c0f [moby#19177]: moby/moby#19177 [moby@96d7db6]: moby/moby@96d7db6 [moby@2f27632]: moby/moby@2f27632 [`imageRouter.postImagesLoad`]: https://github.com/moby/moby/blob/7b9d2ef6e5518a3d3f3cc418459f8df786cfbbd1/api/server/router/image/image_routes.go#L248-L255 Signed-off-by: Sebastiaan van Stijn <[email protected]>
1 parent dafbfc2 commit f594a7f

8 files changed

+20
-36
lines changed

cli/command/image/load.go

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func runLoad(ctx context.Context, dockerCli command.Cli, opts loadOptions) error
7070
if err != nil {
7171
return err
7272
}
73-
defer file.Close()
73+
defer func() { _ = file.Close() }()
7474
input = file
7575
}
7676

@@ -95,12 +95,7 @@ func runLoad(ctx context.Context, dockerCli command.Cli, opts loadOptions) error
9595
if err != nil {
9696
return err
9797
}
98-
defer res.Close()
98+
defer func() { _ = res.Close() }()
9999

100-
if res.JSON {
101-
return jsonstream.Display(ctx, res, dockerCli.Out())
102-
}
103-
104-
_, err = io.Copy(dockerCli.Out(), res)
105-
return err
100+
return jsonstream.Display(ctx, res, dockerCli.Out())
106101
}

cli/command/image/load_test.go

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ func TestNewLoadCommandInvalidInput(t *testing.T) {
7474
assert.ErrorContains(t, err, expectedError)
7575
}
7676

77-
func mockImageLoadResult(content string, json bool) client.ImageLoadResult {
78-
out := client.ImageLoadResult{JSON: json}
77+
func mockImageLoadResult(content string) client.ImageLoadResult {
78+
out := client.ImageLoadResult{}
7979

8080
// Set unexported field "body"
8181
v := reflect.ValueOf(&out).Elem()
@@ -94,31 +94,21 @@ func TestNewLoadCommandSuccess(t *testing.T) {
9494
{
9595
name: "simple",
9696
args: []string{},
97-
imageLoadFunc: func(input io.Reader, options ...client.ImageLoadOption) (client.ImageLoadResult, error) {
98-
// FIXME(thaJeztah): how to mock this?
99-
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader("Success"))}, nil
100-
return mockImageLoadResult(`Success`, false), nil
101-
},
102-
},
103-
{
104-
name: "json",
105-
args: []string{},
10697
imageLoadFunc: func(input io.Reader, options ...client.ImageLoadOption) (client.ImageLoadResult, error) {
10798
// FIXME(thaJeztah): how to mock this?
10899
// return client.ImageLoadResult{
109-
// Body: io.NopCloser(strings.NewReader(`{"ID": "1"}`)),
110-
// JSON: true,
100+
// Body: io.NopCloser(strings.NewReader(`{"ID":"simple","Status":"success"}`)),
111101
// }, nil
112-
return mockImageLoadResult(`{"ID":"1"}`, true), nil
102+
return mockImageLoadResult(`{"ID":"simple","Status":"success"}`), nil
113103
},
114104
},
115105
{
116106
name: "input-file",
117107
args: []string{"--input", "testdata/load-command-success.input.txt"},
118108
imageLoadFunc: func(input io.Reader, options ...client.ImageLoadOption) (client.ImageLoadResult, error) {
119109
// FIXME(thaJeztah): how to mock this?
120-
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader("Success"))}, nil
121-
return mockImageLoadResult(`Success`, false), nil
110+
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader(`{"ID":"input-file","Status":"success"}`))}, nil
111+
return mockImageLoadResult(`{"ID":"input-file","Status":"success"}`), nil
122112
},
123113
},
124114
{
@@ -129,8 +119,8 @@ func TestNewLoadCommandSuccess(t *testing.T) {
129119
assert.Check(t, len(options) > 0) // can be 1 or two depending on whether a terminal is attached :/
130120
// assert.Check(t, is.Contains(options, client.ImageHistoryWithPlatform(ocispec.Platform{OS: "linux", Architecture: "amd64"})))
131121
// FIXME(thaJeztah): how to mock this?
132-
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader("Success"))}, nil
133-
return mockImageLoadResult(`Success`, false), nil
122+
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader(`{"ID":"single-platform","Status":"success"}`))}, nil
123+
return mockImageLoadResult(`{"ID":"single-platform","Status":"success"}`), nil
134124
},
135125
},
136126
{
@@ -139,8 +129,8 @@ func TestNewLoadCommandSuccess(t *testing.T) {
139129
imageLoadFunc: func(input io.Reader, options ...client.ImageLoadOption) (client.ImageLoadResult, error) {
140130
assert.Check(t, len(options) > 0) // can be 1 or two depending on whether a terminal is attached :/
141131
// FIXME(thaJeztah): how to mock this?
142-
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader("Success"))}, nil
143-
return mockImageLoadResult(`Success`, false), nil
132+
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader(`{"ID":"with-comma-separated-platforms","Status":"success"}`))}, nil
133+
return mockImageLoadResult(`{"ID":"with-comma-separated-platforms","Status":"success"}`), nil
144134
},
145135
},
146136
{
@@ -149,8 +139,8 @@ func TestNewLoadCommandSuccess(t *testing.T) {
149139
imageLoadFunc: func(input io.Reader, options ...client.ImageLoadOption) (client.ImageLoadResult, error) {
150140
assert.Check(t, len(options) > 0) // can be 1 or two depending on whether a terminal is attached :/
151141
// FIXME(thaJeztah): how to mock this?
152-
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader("Success"))}, nil
153-
return mockImageLoadResult(`Success`, false), nil
142+
// return client.ImageLoadResult{Body: io.NopCloser(strings.NewReader(`{"ID":"with-multiple-platform-options","Status":"success"}`))}, nil
143+
return mockImageLoadResult(`{"ID":"with-multiple-platform-options","Status":"success"}`), nil
154144
},
155145
},
156146
}
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
Success
1+
input-file: success

cli/command/image/testdata/load-command-success.json.golden

Lines changed: 0 additions & 1 deletion
This file was deleted.
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
Success
1+
simple: success
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
Success
1+
with-comma-separated-platforms: success
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
Success
1+
with-multiple-platform-options: success
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
Success
1+
single-platform: success

0 commit comments

Comments
 (0)