Error out if no validate layers found during unpacking

[coolljt0725]

Signed-off-by: Lei Jitang [email protected]

Instead of silently finished when oci-image-tool create-runtime-bundle unpack a oci-layout to a runtime bundle but there is no valid layer(for example the media type of layer is not correct), it's better error out.

[vbatts]

hmmm. Perhaps an INFO or WARN would be better than error. Folks have discussed the possibility of a manifest that references digests of layers that are not provided. Obviously this implies a distribution or discovery of the layers associated with these digests, but that is a different issue to solve.
As for this, perhaps it is not an error.

[philips]

@vbatts I think it is an error because you can't successfully complete the operation.

[vbatts]

hmm, in the case it seems fine. It just ought not imply an image is invalid for not including the layer referenced in the manifest.

[coolljt0725]

This is just for unpacking an oci image layout to a runtime bundle, I think it's make sense to error out because the rootfs of runtime bundle would be empty if there is no valid layers found.

[philips]

@vbatts yes, for validate I 100% agree. For unpack it must error because it can't complete the operation.

[philips]

I think the error should be like this one: https://github.com/coolljt0725/image-spec/blob/290cb54341fdce2b746a05fc164203843f41dd7e/image/manifest.go#L113

[coolljt0725]

@philips updated

[wking]

On Mon, Sep 05, 2016 at 08:46:08PM -0700, Lei Jitang wrote:

Instead of silently finished when oci-image-tool create-runtime-bundle unpack a oci-layout to a runtime bundle but
there is no valid layer(for example the media type of layer is not
correct), it's better error out.

If the media type was wrong, wouldn't we want to error out here 1?
I don't like silently ignoring media types that we don't understand.

And why is that using MediaTypeImageConfig? Shouldn't it be using
MediaTypeImageSerialization?

The return at the end (outside of the Layers loop) which you touch in
63a71a7 looks like it's the “I've finished successfully unpacking all
the layers” return, and we do want nil there.

[wking]

On Wed, Sep 07, 2016 at 02:41:14AM -0700, W. Trevor King wrote:

And why is that using MediaTypeImageConfig? Shouldn't it be using
MediaTypeImageSerialization?

Ah, this is #257, and yeah, it should be MediaTypeImageLayer.

[coolljt0725]

The return at the end (outside of the Layers loop) which you touch in
63a71a7 looks like it's the “I've finished successfully unpacking all
the layers” return, and we do want nil there.

The current code may be wrong, if there is a valid layer, it will return from https://github.com/opencontainers/image-spec/pull/261/files#diff-66e5333ade2df0ad4720e022ecd0172bL115 so it never go to https://github.com/opencontainers/image-spec/pull/261/files#diff-66e5333ade2df0ad4720e022ecd0172bR120
only no valid layer found or there is no layer it will go to https://github.com/opencontainers/image-spec/pull/261/files#diff-66e5333ade2df0ad4720e022ecd0172bR120

I'll open a new pr to fix this

[jonboulle]

Will need a rework after #274.

[coolljt0725]

@jonboulle yup, will do

[wking]

On Wed, Sep 07, 2016 at 04:03:37AM -0700, Lei Jitang wrote:

The return at the end (outside of the Layers loop) which you touch
in 63a71a7 looks like it's the “I've finished successfully
unpacking all the layers” return, and we do want nil there.

The current code may be wrong…

Your fix in #274 looks good to me.

… only no valid layer found or there is no layer it will go to
https://github.com/opencontainers/image-spec/pull/261/files#diff-66e5333ade2df0ad4720e022ecd0172bR120

More on “no valid layer found” in 1. If ‘layers’ is empty, having
unpack return nil is appropriate, and a successful unpacking will just
leave an empty ‘dest’ directory with arbitrary attributes (owner,
mode, …).