bit unsure about "cannot be pulled in this manner", perhaps

invalid manifest type for image. Use `docker plugin install`

but not really satisfied with that one @sfsmithcha perhaps you have a suggestion?

What about

target is a plugin manifest. Use `docker plugin install` to install plugins.

oh, that looks good, thanks

if m, ok := manifest.(*schema2.DeserializedManifest); ok { m.Manifest.Config.MediaType } ?

I believe only schema2.MediaTypeConfig is expected value here. Wondering if we could just throw an error of an expected configuration type if that is not seen. Ping @aaronlehmann

I think the plugin stuff extends this with an additional media type?

It does, but it has it's own pull method. It is invalid to pull a plugin type here. My preference for the code would be rather than having to have this part of the code know about plugins, just have it check for the configuration types it expects, in this case schema2.MediaTypeConfig

Ah, SGTM

I tried this approach before blacklisting plugins. The problem is that docker pull official Hub images such busybox, python, ubuntu has mediatype set to "application/octet-stream" and not the expected schema2.MediaTypeConfig ("application/vnd.docker.container.image.v1+json").

distribution/distribution#1621 seems relevant.

That we know about. At some point we will have to be restrictive in the supported values. If it fails then we can assess whether it is legitimate and we should make a code change or whether the manifest needs to be recreated. We accept the application/octet-stream because we know there were many manifests mistakenly created with it and that at the time when they were created the only supported value is application/vnd.docker.container.image.v1+json.

For this change in particular, since now there are multiple possible types, I think it makes sense to define what is supported and have a special case for that one distribution bug.

@anusha-ragunathan to me it more about separation of the code. This code should not be pulling plugins, and it really doesn't even need to know about anything that is a non-image. Having the whitelist would also negate the need of having the v0 version defined in distribution since the code change can be isolated to the image pull logic.

We can make these sort of updates later, I am fine with it like this fo rnow

@dmcgowan: distribution knowing about the plugin mediatype should be okay. Are you concerned purely because it is in v0 and will be okay to include it in distribution once we have a stable v1 manifest spec? Also, I agree that code separation is ideal but the above workaround to detecting the container image mediatype feels hackish. What if there are other mediatypes that we are not aware of, but qualify as a good container image today. Then this change will break a lot of users. That's why blacklisting is safe, since it wont break a thing.

I think we need to add something in distribution to handle this sort of filtering and offering a "media type" separate from the version, so we can test to see if the media type is indeed an image rather than something else. I will create an issue in distribution to track this.

I'm surprised golint allowed these not to be commented individually. golint generally requires exported constants to have comments. Are we somehow running golint incorrectly?

golint allows to put one comment on top of const if we want to be lazy (and if there is no comment on any constant).