-
Notifications
You must be signed in to change notification settings - Fork 201
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
load: support buildkit archives #853
load: support buildkit archives #853
Conversation
124b875
to
e04bbd4
Compare
Archives generated with buildkit have some kind of "hybrid" layout which is the same for OCI and Docker archives. OCI ones ship with a manifest.json but set the image's reference in the index.json but in a custom annotation and not the one the OCI image spec wants. Archives in the Docker format set the reference in `RepoTags` of the manifest.json. To support these archives, simply look for the custom containerd annotation *and* change the order back to give OCI archives precedence. Fixes: containers/podman/issues/12560 Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
e04bbd4
to
4076676
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, vrothberg 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 |
Yuck. |
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.
For the record, containers/image#1381 should include the same logic.
@@ -201,15 +216,16 @@ func (r *Runtime) copyFromDefault(ctx context.Context, ref types.ImageReference, | |||
if err != nil { |
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.
Pre-existing: Shouldn’t ociTransport
above have exactly the same naming logic?
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.
That sounds like a good idea
// if index.json has no reference name, compute the image ID instead | ||
if manifestDescriptor.Annotations == nil || manifestDescriptor.Annotations["org.opencontainers.image.ref.name"] == "" { | ||
storageName = nameFromAnnotations(manifestDescriptor.Annotations) | ||
switch len(storageName) { |
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.
Why switch
?
(BTW len(s) == 0
and s == ""
generate exactly the same code, so I’d lean towards the more readable string comparison.)
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.
I have no preference and probably flip-flop between the two depending on the mood
Archives generated with buildkit have some kind of "hybrid" layout which
is the same for OCI and Docker archives. OCI ones ship with a
manifest.json but set the image's reference in the index.json but in a
custom annotation and not the one the OCI image spec wants. Archives
in the Docker format set the reference in
RepoTags
of themanifest.json.
To support these archives, simply look for the custom containerd
annotation and change the order back to give OCI archives precedence.
Fixes: containers/podman/issues/12560
Signed-off-by: Valentin Rothberg rothberg@redhat.com
@containers/podman-maintainers PTAL