manifest: remove ManifestList type #238
Conversation
Signed-off-by: Erik Hollensbe <[email protected]>
| @@ -54,7 +54,7 @@ func GuessMIMEType(manifest []byte) string { | |||
| } | |||
|
|
|||
| switch meta.MediaType { | |||
| case DockerV2Schema2MediaType, DockerV2ListMediaType, imgspecv1.MediaTypeImageManifest, imgspecv1.MediaTypeImageManifestList: // A recognized type. | |||
| case DockerV2Schema2MediaType, DockerV2ListMediaType, imgspecv1.MediaTypeImageManifest: // A recognized type. | |||
There was a problem hiding this comment.
GuessMIMEType no longer guesses mime types from OCI unfortunately. I think we need to get rid of the image manifest type as well.
There was a problem hiding this comment.
Yeah; when OCI has removed the MediaType field, we should have something like this PR whether or not we hide the build breakage by vendoring and old version.
(Or I guess we could leave this in, with hard-coded strings, just to be able to deal with some existing pre-1.0 files.)
| @@ -22,7 +22,6 @@ func TestGuessMIMEType(t *testing.T) { | |||
| mimeType string | |||
| }{ | |||
| {"ociv1.manifest.json", imgspecv1.MediaTypeImageManifest}, | |||
| {"ociv1list.manifest.json", imgspecv1.MediaTypeImageManifestList}, | |||
There was a problem hiding this comment.
you should "rm" the fixture also
|
Ok thanks. Give me a few hours.
…On Mon, Feb 20, 2017 at 2:14 AM Antonio Murdaca ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In manifest/manifest_test.go
<#238 (comment)>:
> @@ -22,7 +22,6 @@ func TestGuessMIMEType(t *testing.T) {
mimeType string
}{
{"ociv1.manifest.json", imgspecv1.MediaTypeImageManifest},
- {"ociv1list.manifest.json", imgspecv1.MediaTypeImageManifestList},
you should "rm" the fixture also
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#238 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABJ6-3G2y1ehCkyspKQxkK_3KxFFWP7ks5reWdXgaJpZM4MFTLv>
.
|
|
This patch isn't right. It wasn't removed, the type was renamed in opencontainers/image-spec#546. But as I mentioned in #193, this is breaking the build because we are trying to run our tests with Since |
|
Right, I've mentioned this before as well. @mtrmac do you mind if I take
another stab at the vendoring requirements? We could vendor out-of-band
just for tests too.
…On Mon, Feb 20, 2017 at 9:10 AM, Aleksa Sarai ***@***.***> wrote:
This patch right. It wasn't *removed*, the type was renamed in
opencontainers/image-spec#546
<opencontainers/image-spec#546>.
But as I mentioned in #193 <#193>,
this is breaking the build because we are trying to run our tests with
master of a specification that is still being worked on. Which is
insanity. The correct fix is to run our tests within a project that has
actually vendored the specification.
Since containers/image still only supports OCI version 1.0.0-rc4 (because
-rc5 isn't out yet) there's no point in making half-changes when we're
going to have to rewrite quite a few parts of the oci: transport.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#238 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABJ6wkegM_9fNdRSZEFNhKR8COKuY_aks5reckVgaJpZM4MFTLv>
.
|
|
@erikh I've started working on fixing it with containers/skopeo#308 -- though it would be great if we didn't require |
|
I agree. I'll see if I can whip up something here soon that will resolve
the issue in a way that makes everyone happy.
…On Mon, Feb 20, 2017 at 9:29 AM, Aleksa Sarai ***@***.***> wrote:
@erikh <https://github.com/erikh> I've started working on fixing it with
containers/skopeo#308
<containers/skopeo#308> -- though it would be
great if we didn't require skopeo in order to test anything (the current
situation with the cross-vendoring is getting a bit silly).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#238 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABJ6xERkD5FloFB3hHk4u-lGLjR0nuGks5rec2MgaJpZM4MFTLv>
.
|
|
I have filed #240 to resolve this issue in another way. I will close this issue now. |
This was breaking in the latest version of the opencontainers manifest library.
I removed it because the type no longer exists, so I presumed the feature no longer exists, perhaps a hasty assumption. I'll research and fix this up if necessary, but it was a very small patch.