Skip to content
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

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

Jamstah
Copy link
Contributor

@Jamstah Jamstah commented Nov 21, 2021

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

@Jamstah Jamstah force-pushed the copy-sparse-manifest branch 2 times, most recently from 204de24 to 3c1f386 Compare November 21, 2021 23:54
Copy link
Contributor

@mtrmac mtrmac left a 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.

cmd/skopeo/copy.go Outdated Show resolved Hide resolved
cmd/skopeo/copy.go Outdated Show resolved Hide resolved
cmd/skopeo/copy.go Outdated Show resolved Hide resolved
cmd/skopeo/copy.go Outdated Show resolved Hide resolved
@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 22, 2021

Do I understand correctly that the primary goal is to provide the “CopyNoImages” value?

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.

@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 22, 2021

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.

Yes, I was probably too hasty deprecating it, its commonly used enough and does no harm.

cmd/skopeo/copy.go Outdated Show resolved Hide resolved
cmd/skopeo/copy.go Outdated Show resolved Hide resolved
docs/skopeo-copy.1.md Outdated Show resolved Hide resolved
docs/skopeo-copy.1.md Outdated Show resolved Hide resolved
docs/skopeo-copy.1.md Outdated Show resolved Hide resolved
docs/skopeo-copy.1.md Show resolved Hide resolved
integration/copy_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mtrmac mtrmac left a 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.)

@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 22, 2021

@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!

@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 23, 2021

Are the tests unstable? These failures don't look related.

integration/test/version Outdated Show resolved Hide resolved
@mtrmac
Copy link
Contributor

mtrmac commented Nov 26, 2021

@vrothberg PTAL when you get a chance.

@mtrmac
Copy link
Contributor

mtrmac commented Nov 26, 2021

@vrothberg PTAL when you get a chance.

… but #1517 should be merged first.

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>
Copy link
Member

@vrothberg vrothberg left a 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?

@mtrmac
Copy link
Contributor

mtrmac commented Nov 29, 2021

there may also be embedded indexes or OCI artifacts (e.g., certain signatures, helm charts, source images, etc.).

copy.Image handles none of these structures right now:

  • skopeo copy is not the right tool for copying a collection of images like a full repo
  • If indices are used to point at signatures (e.g. the Notary v2 “reverse reference” idea), I’d expect skopeo copy to do that transparently as an implementation detail of signatures.
  • (I’m rather unclear on what nested OCI indexes are supposed to mean. It’s undefined/non-interoperable, AFAICT.)
  • What does --index=system mean for helm charts / source images?

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 index-only option value, so it would be consistent, at least.

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.

@vrothberg
Copy link
Member

What does --index=system mean for helm charts / source images?

I don't have a definite answer to that. Maybe a future --index=artifact (possibly with another option to further discriminate).

Note that I do not feel strongly about that but --index would be a minor preference. Extending in the future if needed SGTM

@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 29, 2021

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.

@mtrmac
Copy link
Contributor

mtrmac commented Nov 29, 2021

In the back of my head is a future feature like --multi-arch=[amd64] to do partial copies per-arch, instead of the caller manually scripting multi-arch digest parsing. That works better when the flag name is more constrained. Also --index=system reads weird (but then why would anyone type that, when it’s the default?).

OTOH the one case we actually care about, --index=index-only really isn’t only multi-arch specific, so that would actually be a better name.

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 --index gets that done, I’m perfectly fine with it.

@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 29, 2021

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.

@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2021

I like --multi-arch=[amd64]

@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 30, 2021

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 --multi-arch flag of passing an array is a good one. I'll find time to make the update to containers/image to support it in a future skopeo pr.

@mtrmac
Copy link
Contributor

mtrmac commented Nov 30, 2021

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.

@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 30, 2021

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 :)

@mtrmac
Copy link
Contributor

mtrmac commented Dec 2, 2021

I’ll just make the call so that this gets in: Merging --multi-arch. (There’s still some, unspecified, time left, to change it before the next release.)

@Jamstah thanks again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants