Add Copy.Options.ReportResolvedReference #2609
Conversation
mtrmac
force-pushed
the
copy-resolve-destination
branch
from
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č <[email protected]>
mtrmac
force-pushed
the
copy-resolve-destination
branch
from
f630065 to
1aec455
Compare
mtrmac
force-pushed
the
copy-resolve-destination
branch
2 times, most recently
from
300a42c to
9639152
Compare
... because we will add a parameter named "options". Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This only adds an API method, it does not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
…iners-storage Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
copy-resolve-destination
branch
from
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č <[email protected]>
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@copy-resolve-destination Signed-off-by: Miloslav Trmač <[email protected]>
|
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č <[email protected]>
|
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č <[email protected]>
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.