-
Notifications
You must be signed in to change notification settings - Fork 775
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
Add an option to allow copying image indexes alone #1511
Conversation
204de24
to
3c1f386
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.
Thanks!
Do I understand correctly that the primary goal is to provide the “CopyNoImages” value?
I went with deprecating the --all flag because I thought it was cleaner, but open to suggestions on UX. My other thoughts were to leave them both in and throw an error if they are used together, or to add a
--list-only
to just copy the list.
I think the --all
flag is useful enough, and used frequently enough that we should leave this short option name available as the primary documented approach. Refusing an invalid combination would work well.
WRT --list-only
vs. (some name of) --list-processing=…
, I don’t have a strong preference at this point; adding a single option with alternatives, like this PR does, instead of several top-level options with no obvious relationship to each other, seems a bit more future-proof / easier to scale to new features.
3c1f386
to
ac380b9
Compare
Yes, exactly. I am looking for a solution for mirroring images from official registries for running software offline that are digest-stable but don't need all architectures to be mirrored. |
Yes, I was probably too hasty deprecating it, its commonly used enough and does no harm. |
c41b45f
to
38f148f
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.
(I’d appreciate a second pair of yes on the naming and documentation.)
38f148f
to
2fec2ca
Compare
@mtrmac I also want to say thanks for the specific, thoughtful and speedy reviews (here and over in containers/image), it's always more fun coding with a friend! |
2fec2ca
to
7e22414
Compare
Are the tests unstable? These failures don't look related. |
7e22414
to
3b3dd7d
Compare
@vrothberg PTAL when you get a chance. |
3b3dd7d
to
f5fa6bd
Compare
… but #1517 should be merged first. |
f5fa6bd
to
e5a550f
Compare
The new --multi-arch option allows the user to select between copying the image associated with the system platform, all images in the index, or just the index itself without attempting to copy the images. Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
e5a550f
to
d940154
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.
Code LGTM but I am not convinced by the hardest part: the naming :^)
--multi-arch
restricts an image index to only be used for providing multiple architectures and while they are used like that in most cases, there may also be embedded indexes or OCI artifacts (e.g., certain signatures, helm charts, source images, etc.).
Could we name the flag --index
or --image-index
?
I’m thinking it’s preferable to focus on user-relevant outcomes over internal implementation: “Index” is a somewhat weird jargon for a collection of images, especially it already isn’t a perfect match because it’s not what v2s2 calls the corresponding blob. But then again the code already has an So I don’t feel strongly about this — we can always add one more (mutually exclusive) option. It just feels more natural to me have an ergonomic option for the case we do have, rather than try to predict a universal option for use cases we can’t yet support and don’t fully understand. |
I don't have a definite answer to that. Maybe a future Note that I do not feel strongly about that but |
I'm happy with either, but if we only do multi arch now kind of lean to staying with that. We can design the clean options for a --index when it happens and deprecate this one. |
In the back of my head is a future feature like OTOH the one case we actually care about, Let’s give the US folks a few hours to catch up? But really I’d rather have the PR with any name than delay it until we have perfect clarity, that seems unlikely to happen soon. So if |
Well I'm new here, so will follow guidance of the project. LMK when you think there's enough consensus from maintainers and I'm happy to adjust the PR to suit. |
I like --multi-arch=[amd64] |
We'll need a containers/image PR to add the ability to select architectures, at the moment to copy multiple images you have to specifically provide the digests instead of the architectures: https://github.com/containers/image/blob/main/copy/copy.go#L138 But I agree that the extension to the |
No no no, that was meant just an imagined future feature to motivate a specific approach to naming the current option; I didn’t want to imply that the scope of this work needs to be extended. |
Yes, I meant for a future pr. Its an option I'd find useful, but wasn't planning to try and fit it in here :) |
I’ll just make the call so that this gets in: Merging @Jamstah thanks again! |
This new --list-processing option deprecates the --all option and, when
an image index is copied, allows the user to select between copying the
image associated with the system platform, all images in the index, or
just the index itself without attempting to copy the images.
Signed-off-by: James Hewitt james.hewitt@uk.ibm.com