Add Copy.Options.ReportResolvedReference#2609
Conversation
f524f0d to
f630065
Compare
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@copy-resolve-destination Signed-off-by: Miloslav Trmač <mitr@redhat.com>
f630065 to
1aec455
Compare
300a42c to
9639152
Compare
... because we will add a parameter named "options". Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This only adds an API method, it does not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…iners-storage Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
9639152 to
6ba898f
Compare
|
Note the added commit “HACK: Only return an image ID from ReportResolvedReference for c/storage”. |
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@copy-resolve-destination Signed-off-by: Miloslav Trmač <mitr@redhat.com>
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@copy-resolve-destination Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
This was manually tested using containers/podman#24447 (comment) , and a Podman test is running in containers/podman#24462 . Please review. Cc: @Luap99 |
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@copy-resolve-destination Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
LGTM. I also like this better than reverting. |
| // | ||
| // For the containers-storage: transport, the reference contains an image ID, | ||
| // so that storage.ResolveReference returns exactly the created image. | ||
| ReportResolvedReference *types.ImageReference |
There was a problem hiding this comment.
I find it somewhat confusing that we use the Options field to return information to the caller. But I guess that is needed to avoid breaking changes in the API? I also get the that this is why it needs a pointer to an interface (which itself is also a pointer) but that seems a bit confusing to use as a caller.
Anyway I don't know the code + history here. I trust you that this is the best design
There was a problem hiding this comment.
Yes, we are committed not to change the API.
Alternatively, we could add a copy.ImageWithResolvedReference returning the extra value… and dropping the manifest bytes return value? I don’t know. Most users just don’t need the value.
Sort of hiding this feature in the Options field means we still are committed to API stability, but most users won’t see this and won’t worry about the extra values. And, also, the code implementing the copy can tell whether the caller wants the value or not.
There was a problem hiding this comment.
Yeah I am fine with this, you are the main maintainer here after all and if you like this approach. We don't use the merge bot here, do we? So feel free to merge and move it along into c/common.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This should allow returning precisely the correct image ID from pulls, improving on containers/common#2202 .
To be used in c/common containers/common#2209 .
Absolutely untested at this point.