ImageDestination should receive Writers

[runcom]

@mtrmac just a tracking question

Wouldn't it be better if ImageDestination return Writers? I don't have anything in mind about how to unify all the image destinations to do so but as we return Readers from ImageSource it came to my mind why don't do the same and return Writers there.

I'm saying, why don't we try and further abstract methods in ImageDestination to support this? is it doable? wdyt?

[mtrmac]

I’m not sure. Once you successfully start reading a blob, there is really nothing else to do, so the API can be as simple as a Reader. (Perhaps the underlying storage mechanism may require chunking and splitting the stream into several independent requests, but that would be fairly rare, and completely invisible to the caller.)

For writing, there may well be a “commit” step (e.g. rename() from a temporary name to the final destination for files, a “mark this upload as done” step for the Docker registry API); and perhaps that “commit” should even happen per image, not once per blob. (Compare the cleanup situation in #102 .)

I don’t know, perhaps that can be very cleanly done by using WriteCloser and implementing that commit in Close. It seems to me that using a WriteCloser would result in both more complex callers and more complex implementations right now, though: for PutBlob(…Reader), the caller just calls PutBlob(…GetBlob()). With a Writer, the caller needs to add an extra io.Copy call. And looking at the current implementations:

• dir: and oci: want to do a Sync
• docker:// and openshift: need to check the HTTP status code.

Both are easy to do in a single PutBlob(…Reader); with a WriteCloser we would have to create a separate putBlobProgress object or something.

Right now, it seems to me that just as a refactoring this would make the code just more complex.

---

Other aspects to consider:

• Progress reporting might be much easier if the io.Copy call, or equivalent, were in the copy top-level instead of individual destinations, and there were a Writer. (Right now we would probably have to implement a ProgressReportingReader to fit progress reporting into the API).
• For signature verification, the copy top-level would really like to have a way to tell “yes this layer data matches the hash, commit / no this is all wrong, throw it away” to the ImageDestination. Right now we can just return an error in the Reader.Read and hope that the ImageDestination will delete the blob instead of keeping it around.

So I guess we will eventually need a more complex blob upload API; whether that would be a simple Writer I am not sure. At least the verification aspect will need to be resolved soonish.

[rhatdan]

@runcom @mtrmac @vrothberg THree year old issue, almost. Should we close this?

[mtrmac]

We now do progress reporting, and it did not require an interface change to use io.Writer.

At least as far as skopeo cares, this is an internal implementation details that users don’t care about, and we can change the API as necessary any time, so tracking this in skopeo makes little sense nowadays.

(As far as c/image cares, changing the API would require a major version bump, and anyway if it happens, it should be feature-driven. Right now destinations are easier to implement if they need a “blob commit“ step if they are given an io.Reader and they are allowed to manage the process as necessary; other code structures, e.g. some kind of helper, might be possible, but without a clear reason to restructure this, let’s not worry about it.)