Add the mediaType to the error #30047

duglin · 2017-01-11T03:31:29Z

Without this fix the error the client might see is:
target is unknown
which wasn't helpful to me when I saw this today. With this fix I
now see:
MediaType is unknown: 'text/html'
which helped me track down the issue to the registry I was talking to.

Signed-off-by: Doug Davis dug@us.ibm.com

AkihiroSuda · 2017-01-11T14:40:57Z

This test needs to be updated

00:42:29.420 ----------------------------------------------------------------------
00:42:29.421 FAIL: docker_cli_plugins_test.go:173: DockerRegistrySuite.TestPluginInstallImage
00:42:29.421 
00:42:29.421 docker_cli_plugins_test.go:184:
00:42:29.421     c.Assert(out, checker.Contains, "target is image")
00:42:29.421 ... obtained string = "Error response from daemon: MediaType is image: \"application/vnd.docker.container.image.v1+json\"\n"
00:42:29.422 ... substring string = "target is image"
00:42:29.422 
00:42:32.844 
00:42:32.844 ----------------------------------------------------------------------

aaronlehmann · 2017-01-11T22:34:34Z

The CLI also expects the errors to have the current format:

https://github.com/docker/docker/blob/de54284bd7cf4e1c8c255c61d0cf05533f3c2ba4/cli/command/image/pull.go#L77
https://github.com/docker/docker/blob/de54284bd7cf4e1c8c255c61d0cf05533f3c2ba4/cli/command/plugin/install.go#L167

duglin · 2017-01-12T03:27:56Z

@aaronlehmann fixed

aaronlehmann · 2017-01-13T18:13:20Z

cli/command/image/pull.go

Perhaps the CLI should strip the mediatype here (and in the plugin case) to make the user-visible error less verbose.

I agree that having the mediatype in the general case is useful.

dmcgowan · 2017-01-23T23:20:22Z

Can we focus this PR on just changing the message to include the media type when target is unknown, so
target is unknown to target is unknown: text/html, changing target to MediaType is unnecessary and doesn't provide any better understanding, and including the media type when it is known and already properly interpreted also provides no help to the user. If we think the media type is potential useful for debugging then we can add a debug log.

stevvooe · 2017-01-23T23:29:42Z

I'd prefer we don't say the word "media type" to users.

duglin · 2017-01-29T23:59:06Z

@stevvooe what would like it to say instead?

stevvooe · 2017-01-30T19:37:48Z

@duglin It said target. The current error message doesn't even make sense: "MediaType is image" is a false statement.

duglin · 2017-01-31T20:07:12Z

@stevvooe see if you're ok with the new stuff

stevvooe · 2017-01-31T21:36:32Z

integration-cli/docker_cli_plugins_test.go

Aren't we fetching a plugin here?

@stevvooe yea that is kind of funky - I tweaked it - see if this new one is any better.

stevvooe · 2017-01-31T21:40:29Z

@duglin Looks fine.

There might be an inconsistency in the test. Added a comment.

stevvooe · 2017-02-01T00:43:33Z

LGTM

vieux · 2017-02-01T02:29:58Z

ping @tiborvass

vieux · 2017-02-14T01:02:39Z

LGTM ping @tiborvass @anusha-ragunathan

aaronlehmann · 2017-02-14T01:21:56Z

cli/command/image/pull.go

Is this correct? It doesn't seem consistent with the error generated by pullV2Tag.

anusha-ragunathan · 2017-02-14T02:15:34Z

#30047 (comment)

I agree with @dmcgowan . Users dont need to be confused with MediaType info, when a target is known.

vieux · 2017-02-28T18:08:30Z

@duglin what do you think about the last comments ?

duglin · 2017-02-28T18:11:21Z

my concern with the word "target" is that is doesn't help someone know what's going on. "target" of what? At least "MediaType", to me, would indicate that some field in the metadata about the image is wrong. I would never have guess that "target" would refer to a field in the image.

anusha-ragunathan · 2017-02-28T18:32:07Z

target is not a field in the image/plugin. It is the remote that the user is referring to.
remote is also an appropriate term. eg. remote is plugin or remote is image

duglin · 2017-02-28T19:34:06Z

ok, I can live with "remote" more than "target" :-) will udpdate

Without this fix the error the client might see is: target is unknown which wasn't helpful to me when I saw this today. With this fix I now see: MediaType is unknown: 'text/html' which helped me track down the issue to the registry I was talking to. Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2017-03-08T16:49:37Z

ping @tiborvass @anusha-ragunathan for a final LGTM I think

anusha-ragunathan · 2017-03-11T00:43:05Z

LGTM

tiborvass · 2017-03-11T00:44:44Z

LGTM

Add the mediaType to the error

GordonTheTurtle added the status/0-triage label Jan 11, 2017

AkihiroSuda added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Jan 11, 2017

duglin force-pushed the distError branch from 8a020f6 to 981608f Compare January 11, 2017 14:44

duglin removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jan 11, 2017

duglin force-pushed the distError branch from 981608f to 2d3b45a Compare January 12, 2017 03:27

aaronlehmann reviewed Jan 13, 2017

View reviewed changes

aaronlehmann mentioned this pull request Jan 23, 2017

distribution: Add text/html and application/json as image mediatypes #30262

Merged

duglin force-pushed the distError branch from 2d3b45a to ca6fa74 Compare January 31, 2017 16:20

stevvooe reviewed Jan 31, 2017

View reviewed changes

duglin force-pushed the distError branch from ca6fa74 to c2aae79 Compare January 31, 2017 23:41

GordonTheTurtle assigned vieux Feb 10, 2017

aaronlehmann reviewed Feb 14, 2017

View reviewed changes

cli/command/image/pull.go Outdated

Copy link

aaronlehmann Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? It doesn't seem consistent with the error generated by pullV2Tag.

duglin force-pushed the distError branch from c2aae79 to c127d96 Compare February 28, 2017 19:36

tiborvass merged commit e10f9db into moby:master Mar 11, 2017

GordonTheTurtle added this to the 17.04.0 milestone Mar 11, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30047 from duglin/distError

2881566

Add the mediaType to the error

This was referenced Aug 26, 2025

cli/command/image: remove special handling for plugin errors on pull docker/cli#6374

Merged

[c8d] improve errors when pulling a plugin instead of image #50818

Open

thaJeztah mentioned this pull request Sep 8, 2025

cli/command/plugin: remove special error handling on install, upgrade docker/cli#6439

Merged

Add the mediaType to the error #30047

Add the mediaType to the error #30047

Uh oh!

Conversation

duglin commented Jan 11, 2017

Uh oh!

AkihiroSuda commented Jan 11, 2017

Uh oh!

aaronlehmann commented Jan 11, 2017

Uh oh!

duglin commented Jan 12, 2017

Uh oh!

aaronlehmann Jan 13, 2017

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Jan 23, 2017

Uh oh!

stevvooe commented Jan 23, 2017

Uh oh!

duglin commented Jan 29, 2017

Uh oh!

stevvooe commented Jan 30, 2017

Uh oh!

duglin commented Jan 31, 2017

Uh oh!

stevvooe Jan 31, 2017

Choose a reason for hiding this comment

Uh oh!

duglin Jan 31, 2017

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Jan 31, 2017

Uh oh!

stevvooe commented Feb 1, 2017

Uh oh!

vieux commented Feb 1, 2017

Uh oh!

vieux commented Feb 14, 2017

Uh oh!

aaronlehmann Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

anusha-ragunathan commented Feb 14, 2017

Uh oh!

vieux commented Feb 28, 2017

Uh oh!

duglin commented Feb 28, 2017

Uh oh!

anusha-ragunathan commented Feb 28, 2017

Uh oh!

duglin commented Feb 28, 2017

Uh oh!

duglin commented Mar 8, 2017

Uh oh!

anusha-ragunathan commented Mar 11, 2017

Uh oh!

tiborvass commented Mar 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants