Skip to content

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 [email protected]

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

@Jamstah Jamstah force-pushed the copy-sparse-manifest branch from 3c1f386 to ac380b9 Compare November 22, 2021 15:17
@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.

@Jamstah Jamstah force-pushed the copy-sparse-manifest branch from c41b45f to 38f148f Compare November 22, 2021 16:05
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 Jamstah force-pushed the copy-sparse-manifest branch from 38f148f to 2fec2ca Compare November 22, 2021 17:15
@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 Jamstah force-pushed the copy-sparse-manifest branch from 2fec2ca to 7e22414 Compare November 22, 2021 22:07
@Jamstah
Copy link
Contributor Author

Jamstah commented Nov 23, 2021

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

@mtrmac
Copy link
Contributor

mtrmac commented Nov 26, 2021

@vrothberg PTAL when you get a chance.

@Jamstah Jamstah force-pushed the copy-sparse-manifest branch from 3b3dd7d to f5fa6bd Compare November 26, 2021 11:51
@mtrmac
Copy link
Contributor

mtrmac commented Nov 26, 2021

@vrothberg PTAL when you get a chance.

… but #1517 should be merged first.

@Jamstah Jamstah force-pushed the copy-sparse-manifest branch from f5fa6bd to e5a550f Compare November 27, 2021 13:44
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 <[email protected]>
@Jamstah Jamstah force-pushed the copy-sparse-manifest branch from e5a550f to d940154 Compare November 27, 2021 15:38
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