This is ugly and suprising for an ImageSource to do (consider a caller who does not expect to use image.Image). Can we instead add the v2s1→OCI conversion? It might well be possible to do by creating a temporary v2s2 memoryImage (using the existing code) and then using the existing v2s2→OCI conversion.

(We haven’t supported a v2s1→OCI conversion before, so AFAICT this is not an essential part of this PR.)

makes sense to have v2s1 -> OCI at this point, so yeah, I'll implement this.

Considering the failing tests, could we merge a version without that conversion now, and deal with the v2s1→OCI conversion as a separate issue or PR?

sure

wait, tests will fail w/o this mime type check, shall I leave this here?

What is the failure / code path?

basic skopeo copy docker://busybox oci:something, we need this to ask for at least v2s2 if we want OCI conversion (since we cannot convert from v2s1 which is the default when the registry doesn't understand the media type, see the comment, the test failure is in skopeo tests)

What is the failure / code path?

Let me guess: ociDest.SupportedManifestMIMETypes returns OCI only, and so copy.Image asks for unsupported manifest MIME type, and that gets sent to the registry.

Adding the OCI conversion is the long-term fix.

Short-term, can we:

• move the array from dockerImageDestinationSupportedManifestMIMETypes into a global variable (shared by src and dest)
• in newImageSource, ignore requestedManifestMIMETypes if it has no value common with the known supported MIME types (as defined by the variable from above)?

That’s still kind of a hack, but at least it is conceptually clean: dockerImage* knows what it supports/accepts, and does not need to know about OCI.

(*shrug*, if you want to merge it with the existing hack instead, feel free. Just please make sure to file an issue to have this cleaned up in the future.)

Is this really necessary to read the config here? It would perhaps be preferable to preserve the ImageSource and pass a nil configBlob, the way manifestOCI1.convertToManifestSchema2 does it.

This …NonDistributable conversion is not present in the new conversion code, is that intentional?

I missed it somehow

fixed

I am tempted no to try at all… But if we should, I think it would be much cleaner to have the caller supply a mimeType as a parameter than guessing by parsing contents in an unknown format in here.

let's skip this altogether for now and re-iterate on adding an optional mime type, but then, ppl might cheat on mime type as well no?

A lying caller will get what is coming to it :)

Right now we do do this wrong, because copy.determineManifestConversion can decide to keep an unsupported type anyway (it is treating SupportedManifestMIMETypes as a best-effort request), and PutManifest isn’t told about that. But we would be sending the correct MIME type to PutManifest if there was a way to do that.

Also, docker_schema2.go has (unlike …schema1.go) a reasonable test coverage, and it would be nice to preserve that (the conversions happen frequently enough for users to care but perhaps not frequently enough to prevent us from breaking them without noticing). This should be just a pretty trivial variant of TestConvertToManifestSchema2.

yup, have it on my todo, just wanted to bring this PR up for general consensus on the way I'm fixing this.

I think a map[string]struct{} is more idiomatic, but I really don’t care enough.

arg, that's how it was, then changed my mind, I'll put it back with struct{}

on a second though, I like comparing with just ! - we change this any time though...

Sure, whatever works.

This is still O(N^2): move the call supportedManifestMIMETypesMap outside the loop, save it in a variable. Then it is O(N).

(Or don’t bother and just write two nested loops. The arrays are small enough.)

This is still O(N^2): move the call supportedManifestMIMETypesMap outside the loop, save it in a variable. Then it is O(N).

silly me, ETOOLATE

This would ignore the input value if any value is unrecognized; I think it should ignore input only if all values are unrecognized (so that a caller can say “i prefer v2s1, then OCI, then v2s2 as a last resort”.)

break here, and then it’s more natural to reword as if mts[mtrequested] { EDIT reset = false;break }.

(and as long you are changing this) this would benefit from either clearer variable names, or a comment explicitly describing what this loop intends to do. Preferably the former, perhaps naming them supportedMIMEs and acceptableRequestedMIMEs (with the opposite semantics to reset, starting as false).

…also, no need for the var() here AFAICS, just use :=. Or does that have an idiomatic meaning?

This rejects requestedManifestMIMETypes if any content is unrecognized; shouldn’t it reject them only if none are recognized?

(i.e. when reaming reset to acceptableRequestedMIMEs, only the last if was flipped AFAICS.)

I believe I've fixed it now - sorry I'm in a meeting but want to get this in asap to unblock other PRs

I was thinking

	supportedMIMEs := supportedManifestMIMETypesMap()
	acceptableRequestedMIMEs := false
	for _, mtrequested := range requestedManifestMIMETypes {
		if supportedMIMEs[mtrequested] {
			acceptableRequestedMIMEs = true
			break
		}
	}
	if !acceptableRequestedMIMEs {
		requestedManifestMIMETypes = manifest.DefaultRequestedManifestMIMETypes
	}

Perhaps I am being an idiot?

fixed and force pushed

This works for now (i.e. at it was perfectly valid a week ago), longer-term we should actually know whether the input is gzipped. Filed #229.