manifest create: add --amend and --replace for non-list images#6676
manifest create: add --amend and --replace for non-list images#6676c-kruse wants to merge 2 commits intocontainers:mainfrom
Conversation
dosubot
bot
added
the
size:L
|
@c-kruse Could you please sign your commits? https://github.com/containers/buildah/pull/6676/checks?check_run_id=62867948061 |
Extend `manifest create` to handle name conflicts with non-list images: - --amend: convert a non-list image into a manifest list with the original image as its first entry. - --replace: remove the name from the existing image before creating a new manifest list. Closes: containers#6639 Signed-off-by: Christian Kruse <christian@c-kruse.com>
c-kruse
force-pushed
the
manifest-create-amend-replace
branch
from
89d9abc to
83b1bfd
Compare
|
All kinds of test unhappiness, but at least some of it is not related to this PR. Restarting tests. |
Co-authored-by: Tom Sweeney <tsweeney@redhat.com> Signed-off-by: Christian Kruse <christian@c-kruse.com>
nalind
left a comment
nalind
left a comment
There was a problem hiding this comment.
Oh, nice, this fixes the rest of https://issues.redhat.com/browse/RUN-2047.
LGTM with one optional suggestion.
| img, _, lookupErr := runtime.LookupImage(listImageSpec, &libimage.LookupImageOptions{ManifestList: true}) | ||
| if lookupErr != nil { | ||
| return fmt.Errorf("looking up image to amend %q: %w", listImageSpec, lookupErr) | ||
| } |
There was a problem hiding this comment.
It's not a problem being introduced by this PR, but coming back to it, I think I would have wanted to have a confidence check here that the image that LookupImage() handed back to us did indeed have at least some overlap between its names and the elements of names. But this is definitely an improvement over the earlier version, and that's not a blocker.
There was a problem hiding this comment.
Hi @nalind. I was wondering about this, and did do a little dive into ExpandNames + how LookupImage works. I'm not sure I understand it well enough to improve much here.
My impression is that names here (if no err) will always have length == 1, names[0] may not equal listImageSpec, and that it may not be strictly true that runtime.LookupImage/runtime.LookupManifestList will return the same image for names[0] and listImageSpec. I'm not sure if there's a most correct way to resolve an image name between the two, or if like you suggested it's better to just exit with an error when there is a mismatch.
4 participants
Extend
manifest createto handle name conflicts with non-list images:Closes: #6639
What type of PR is this?
/kind feature
Special notes for your reviewer:
Some of the original
manifest create --amendlogic did not make total sense to me and has been omitted. I suspect there could be tricky edge cases I haven't considered that may warrant some of that debug logging.Does this PR introduce a user-facing change?
Yes.