Make UpdatedImage update after conversion of MIME type

[lumjjb]

Addressing Issue #746

Signed-off-by: Brandon Lum lumjjb@gmail.com

[lumjjb]

CI passed (https://travis-ci.org/containers/image/builds/657930395?utm_source=github_status&utm_medium=notification), pushing formatting nits

[mtrmac]

Thanks!

(Non-blocking: Are you interested in writing tests for the newly supported features as well?)

[mtrmac]

I’m not thrilled with dropping these checks — confused callers should be detected.

[lumjjb]

I was looking at this and there's some inconsistency with docker schema v1 where it does not have this check.. Is there a particular reason for it? Or should I change it to reflect confused callers as well?

https://github.com/containers/image/blob/master/image/docker_schema1_test.go#L362-L369

[mtrmac]

The trouble in there is that the manifest looks the same for both MIME types, i.e. by the time manifestSchema1FromManifest is called, we can’t tell whether it is a bug trying to convert to “the same” MIME type or a generic caller just trying the next MIME type in the list without understanding that the two are really not different. (In particular, see how determineManifestConversion works with dockerImageDestination.SupportedManifestMIMETypes.)

Overall, I do think this check is valuable to detect mistakes, but not so valuable that it’s worth forcing every generic caller to understand this schema1 idiosyncrasy.

[lumjjb]

Ahh!! okay. Gotcha. Thanks, that comment in the schema1 file makes a lot more sense with this context!

[mtrmac]

And it would be nice to have the “no conversion” edits (LayerInfos+EmbeddedDockerReference) visually/structurally separated from the “convert depending on MIME type” path. Doing that without duplicating the “no conversion needs to happen” check rather calls for a helper to do the conversion + UpdatedImage call (and that helper could drop the ManifestMIMEType edit request so that unexpected no-ops are detected, see elsewhere).

Maybe

converted, err := convertManifestIfRequired(options, map[string]func(context.Context, types.ManifestUpdateInformation)  (types.Image, error) {
    manifest.DockerV2Schema2MediaType: copy.convertToManifestSchema2,
    imgspecv1.MediaTypeImageManifest:  copy.convertToOCI, // a new helper
})
if err != nil { fail }
if converted != nil { return converted }
// ordinary edits for LayerInfos and EmbeddedDockerReference follow

with a

func convertManifestIfRequired(options types.ManifestUpdateOptions, converters map[string]func(context.Context, types.ManifestUpdateInformation)  (types.Image, error) {
    if options.ManifestMIMEType != "" {
        if converter, ok := converters[options.ManifestMIMEType]; ok {
            tmp, err := converter(options.InformationOnly)
            if err != nil { fail }
            optionsCopy := options
            optionsCopy.ManifestMIMEType = ""
            return tmp.UpdatedImage(ctx, optionsCopy)
        }
    }
    return nil, nil
}

[lumjjb]

Yea this makes sense. Let me make this change.

[lumjjb]

Am updating incrementally. Will ping you when ready for review again

[lumjjb]

@mtrmac ready for another look!

[lumjjb]

Rebased

[mtrmac]

Aren’t the non-conversion edits now applied twice on conversions (once before the convertManifestIfRequired call, and once inside it)?

[mtrmac]

Aren’t the non-conversion edits now applied twice on conversions (once before the convertManifestIfRequired call, and once inside it)?

This admittedly doesn’t matter now, strictly speaking, because ManifestUpdateOptions are more or less idempotent — but there are some RFEs floating around for more editing functionality (“add a layer”) that should not be applied twice, and just being clear about the order of operations should make it easier to make future changes.

[lumjjb]

Thanks for the comments @mtrmac changes made!

(edit keyboard cat)

[lumjjb]

Rebased! All comments should be addressed.

[mtrmac]

LGTM. Thanks!

@vrothberg PTAL.

[lumjjb]

Apparently github suggestions feature doesn't add signature. repushed..

[lumjjb]

rebased

[lumjjb]

rebased. @vrothberg any chance you've managed to take a look at this?

[vrothberg]

rebased. @vrothberg any chance you've managed to take a look at this?

Thanks for the ping! I will have a look first thing tomorrow morning.

[vrothberg]

LGTM, thanks @lumjjb!

Could you squash the commits into one before merging?

[lumjjb]

@vrothberg done! Thanks!