-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
podman load/save: support multi-image docker archive #6811
podman load/save: support multi-image docker archive #6811
Conversation
Need to get containers/image#975 in before. |
cda54b9
to
9efd4e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few notes while primarily reading the called c/image implementation.
9efd4e8
to
34bca45
Compare
aef021b
to
e745b41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to libpod/image/pull.go
were dropped — is that intentional?
In a sense they really don’t belong into this PR, OTOH fixing the existing fairly broken podman pull docker-archive:…
code would be nice.
That was intentional as I want us to focus on the multi-image part first. Once that's in, we can tackle the next issue - unless you feel strongly to tackle both at once. |
I’m fine with having the PR narrowly scoped; what is/isn’t necessary to keep the Podman repo maintainable is up to the judgment of Podman maintainers. |
e745b41
to
22a6361
Compare
22a6361
to
00c1f5a
Compare
40a2408
to
0a03352
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More skeletons in the closet…
0041d39
to
b1a155f
Compare
064ce90
to
f06bea3
Compare
Back to c/storage v1.23.0 |
@edsantiago are https://github.com/containers/podman/pull/6811/checks?check_run_id=1007722626 flakes? @mtrmac, I am running out of time. If you find some cycles, feel free to take over this PR and push it over the finish line. |
@vrothberg those show every sign of being #7195 but I've never seen so many in one run. |
Restarted the last flaky job. The flakes are hitting this PR hard. |
Your getting hit by the flake I can not get by. |
8bd896e
to
0c31a70
Compare
It's green, it's green! @rhatdan, let's merge before the CI gods stop shining upon me head. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK (full review now), just some maintainability suggestions.
Also, this vendors an unreleased version of c/image. Is that OK? |
OK for now, but we'll need to get a c/image release out in the next several weeks for Podman 2.1.0 so we don't cut a full release with a prerelease c/image |
Support loading and saving tarballs with more than one image. Add a new `/libpod/images/export` endpoint to the rest API to allow for exporting/saving multiple images into an archive. Note that a non-release version of containers/image is vendored. A release version must be vendored before cutting a new Podman release. We force the containers/image version via a replace in the go.mod file; this way go won't try to match the versions. Signed-off-by: Valentin Rothberg <[email protected]>
0c31a70
to
7fea467
Compare
Final polish based on @mtrmac's comments pushed. |
k8s.io/api v0.0.0-20190620084959-7cf5895f2711 | ||
k8s.io/apimachinery v0.19.0 | ||
k8s.io/client-go v0.0.0-20190620085101-78d2af792bab | ||
) | ||
|
||
replace github.com/containers/image/v5 => github.com/containers/image/v5 v5.5.2-0.20200902171422-1c313b2d23e0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a WIP until this is released? I think we should release containers/image so that @mheon can release pdoman 2.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mheon acked above (see #6811 (comment)). We just need to release before v2.1.0 which is still a bit in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
/lgtm |
Is there any follow up work planned to bring the other formats into line with this? In particular I'm interested in |
The @nalind @mtrmac I haven't checked the code but assume that the |
A similar list/get-reference API seems natural; the thing I’m not currently sure about is multi-arch support; that somehow uses OCI indices, and I’m not quite sure how. IIRC it’s not a nested-index design, so the top-level index might be a multi-arch image set, not a multi-image archive. @nalind probably knows more. |
When we merged manifest list support, the Copying using
Copying from another location into the same
or
When using an If there's more than one item in the |
Fixes: #2669
Signed-off-by: Valentin Rothberg [email protected]