[WIP] Add multi manifest oci-archive support #921
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: umohnani8 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
vrothberg
left a comment
vrothberg
left a comment
There was a problem hiding this comment.
Just a very quick skim. It seems the commit is reverting some previous ones.
umohnani8
force-pushed
the
multi
branch
4 times, most recently
from
5b1833d to
c0f3141
Compare
umohnani8
force-pushed
the
multi
branch
2 times, most recently
from
c811431 to
1331f03
Compare
umohnani8
force-pushed
the
multi
branch
3 times, most recently
from
d4a38e5 to
f9d3e66
Compare
umohnani8
force-pushed
the
multi
branch
2 times, most recently
from
985c8a5 to
8779bc6
Compare
umohnani8
force-pushed
the
multi
branch
2 times, most recently
from
71a2345 to
219ab4e
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return r.copyFromOCIArchiveReader(ctx, reader, options) |
There was a problem hiding this comment.
If readerRef points at specific image, this seems to completely lose that choice.
Cc: @vrothberg . I thought the distinction is that podman pull oci-archive:… always pulls exactly the one image that c/image uses under that reference, while podman load can possibly pull all images from an archive.
(Compare also the discussion about NewReaderForReference / LoadManifestDescriptor in the c/image PR.)
There was a problem hiding this comment.
Cc: @vrothberg . I thought the distinction is that podman pull oci-archive:… always pulls exactly the one image that c/image uses under that reference, while podman load can possibly pull all images from an archive.
Yes, podman pull expects one image. You can check out the behavior via podman pull docker-archive:libimage/testdata/docker-two-images.tar.xz
Signed-off-by: Urvashi Mohnani <[email protected]>
umohnani8
force-pushed
the
multi
branch
2 times, most recently
from
302dfba to
c9cd8ba
Compare
| } | ||
| defer func() { | ||
| if err := reader.Close(); err != nil { | ||
| logrus.Errorf(err.Error()) |
There was a problem hiding this comment.
.Error() is not a format string; this should be something like logrus.Errorf("%s", err.Error())
|
@umohnani8: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Urvashi Mohnani <[email protected]>
|
Can we close this one due to lack of activity? |
4 participants
Pulling in containers/image#1381 and adding it to the load and save logic here.