Fix saving in oci format #6562
Fix saving in oci format #6562
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
@mtrmac PTAL |
|
LGTM, but would like an ack from @mtrmac |
|
@mheon This is going to blow up because loading from an OCI-DIR image seems to be broken. I am not sure of the correct behaviour though. |
| @@ -1475,7 +1476,8 @@ func (i *Image) Save(ctx context.Context, source, format, output string, moreTag | |||
| return errors.Wrapf(err, "error getting OCI archive ImageReference for (%q, %q)", output, destImageName) | |||
| } | |||
| case "oci-dir": | |||
| destRef, err = directory.NewReference(output) | |||
| destImageName := imageNameForSaveDestination(i, source) | |||
| destRef, err = layout.NewReference(output, destImageName) | |||
There was a problem hiding this comment.
(Preserving the // destImageName may be "" comment would be consistent, and not lead the readers to wonder why it can be so for one and not the other.)
test/e2e/save_test.go
Outdated
| @@ -75,7 +75,7 @@ var _ = Describe("Podman save", func() { | |||
| Expect(save).To(ExitWithError()) | |||
| }) | |||
|
|
|||
| It("podman save to directory with oci format", func() { | |||
| It("podman save and load to directory with oci format", func() { | |||
There was a problem hiding this comment.
Looking at the test failures:
- At least going by descriptions without digging into PR history, it’s not clear that all of them wanted to use the OCI directory format; maybe they intended to use
docker-dir. podman loaddoes not implement EDIToci-archiveoci(https://github.com/containers/libpod/blob/091b9b1163ecbc3e64a9152b5e57e969b4475c2a/libpod/runtime_img.go#L257 ),podman pullEDIToci-archiveoci:…might work in the meantime.
There was a problem hiding this comment.
It’s not entirely clear that podman load should try all of these formats — but probably so (and leaving podman pull for the remote pulls and the explicit transport:… uses). It might be worth checking whether the input path is a directory instead of trying all four formats every time.
There was a problem hiding this comment.
The error I am seeing is a lack of manifest.json file?
# /bin/podman save --format docker-dir -o /tmp/docker-dir alpine
# ./bin/podman save --format oci-dir -o /tmp/oci-dir alpine
# ls /tmp/docker-dir
50644c29ef5a27c9a40c393a73ece2479de78325cae7d762ef3cdc19bf42dd0a blobs manifest.json version
a24bb4013296f61e89ba57005a7b3e52274d8edd3ae2077d04395f806b63d83e index.json oci-layout
# ls /tmp/oci-dir
blobs index.json oci-layout
Should there be a manifest.json file in the oci-dir?
There was a problem hiding this comment.
There shouldn’t be a manifest.json. The missing manifest.json error is reported by a dir: source — as mentioned above, podman load tries to use dir: but not oci: (I incorrectly said oci-archive:, which is tried).
Signed-off-by: Daniel J Walsh <[email protected]>
| if err != nil { | ||
| return errors.Wrapf(err, "error getting directory ImageReference for %q", output) | ||
| return errors.Wrapf(err, "error saving the oci directory ImageReference for (%q, %q)", output, destImageName) |
There was a problem hiding this comment.
This is not quite right — actual saving happens in PushImageToReference below.
| return errors.Wrapf(err, "error saving the oci directory ImageReference for (%q, %q)", output, destImageName) | |
| return errors.Wrapf(err, "error getting OCI directory ImageReference for (%q, %q)", output, destImageName) |
|
@QiWang19 Could you take this one over. The issue here is on the load side not on the save. |
github-actions
bot
added
the
locked - please file new issue/PR
Fixes: #6544
Signed-off-by: Daniel J Walsh [email protected]