copy/single: accept custom `*Options` and wrap arguments in `copySingleImageOptions` #2050

flouthoc · 2023-07-21T08:23:56Z

After #2048 there is no room for copy/multiple to pass custom options to copySingleImage while processing each instance, following PR introduces that functionality again and wraps options to simpler struct.

Needed by: #1987

flouthoc · 2023-07-21T08:24:51Z

@mtrmac @vrothberg PTAL

mtrmac · 2023-07-21T17:44:26Z

copy/single.go

@@ -30,6 +30,7 @@ import (
 // imageCopier tracks state specific to a single image (possibly an item of a manifest list)
 type imageCopier struct {
 	c                             *copier
+	options                       *Options


copier already contains an options pointer. Why is a separate option struct passed around now?

If imageCopier has two sets of options, there should be a very good documentation for when to use which ones.

I guess that two options pointers are unnecessary; if imageCopier should deviate from the global options in some way, make that deviation a part of the new singleImageOpts.

I’m sorry, the motivation was clearly explained in a commit message.

Yes, in that case, let’s make only the compression format difference new fields in copySingleImageOptions, instead of a full copy, with the intent ambiguities (and, much less important, the overhead of having to make a copy of the large copy.Options struct).

Should copySingleImageOptions include only compression fields or entire DestinationCtx, for instance see copy/single.go its using DestinationCtx directly at lot of places, I'm thinking of including a custom DestinationCtx in copySingleImageOptions and then in imageCopier as well so other helpers can use it.

I am not sure if DestinationCtx should reflect custom compression at all places in copy/single.go or not. Maybe I need to verify that ?

Nvm I think DestinationCtx is not needed only compression should be enough.

Just the compression parameters, I think. Practically, copy/multiple.go does not need to create a copy of DestinationCtx just like it shouldn’t need to create a copy of Options.

It does make sense to reason that if SystemContext contains compression choices, and we are passing SystemContext to transports, then we should update the SystemContext with the chosen compression — but transports do not care (and must not care! they must write the data in PutBlob without modification for the manifest to be valid); SystemContext.Compression… is only used in c/image/copy, so creating an updated clone is, I think, unnecessary.

The history is that c/image was originally just code in a Skopeo CLI tool, where IIRC CLI parameters were just global variables. When we split c/image into a separate project, we shuffled those values to a single global SystemContext rather than restructure the option passing completely.

Nowadays, basically SystemContext exists primarily so that c/image/copy can be generic code which doesn’t know much about transports options, and callers can still set options for the transports; I’m trying to (very slowly) move isolated c/image subpackages away from using SystemContext for options, compare e.g. signature/simplesigning[1]. In that sense, passing the compression choice in DestinationCtx and not directly in Options is plainly not something we would do nowadays.

[1] (but also, see how much more code doing it the modern way is, compared to just adding a field to SystemContext)

copy/single.go

mtrmac · 2023-07-21T17:49:55Z

copy/copy.go

@@ -251,7 +251,11 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef,

 	if !multiImage {
 		// The simple case: just copy a single image.
-		single, err := c.copySingleImage(ctx, c.unparsedToplevel, nil, false)
+		singleImageOpts := copySingleImageOptions{unparsedImage: c.unparsedToplevel,


Non-blocking: In the three callers, I think inlining the struct constructor would be a tiny bit more readable — it would be clear that the data is single-use and readers wouldn’t have to worry about future uses of singleImageOpts.

flouthoc · 2023-07-21T18:46:10Z

I'll rebase on final review.

copy/single.go

copy/copy.go

copy/single.go

Create a wrapper around arguments of `copySingleImage` Signed-off-by: Aditya R <arajan@redhat.com>

After containers#2048 there is no room for copy/multiple to pass custom compressionFormat to copySingleImage while processing each instance, following PR introduces that functionality again and wraps options to simpler struct. Signed-off-by: Aditya R <arajan@redhat.com>

If its a regular copy callers might not set `compressionFormat` and `compressionLevel` in such case still honor compression which is set in DestinationCtx from copier. Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac

Thanks! LGTM.

mtrmac · 2023-07-21T19:25:27Z

copy/single.go

@@ -40,6 +40,12 @@ type imageCopier struct {
 	requireCompressionFormatMatch bool
 }

+type copySingleImageOptions struct {
+	requireCompressionFormatMatch bool
+	compressionFormat             *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil.


Suggested change

compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil.

compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the caller explicitly requested one, or nil.

for future reference

mtrmac · 2023-07-21T19:25:55Z

copy/single.go

+type copySingleImageOptions struct {
+	requireCompressionFormatMatch bool
+	compressionFormat             *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil.
+	compressionLevel              *int


Suggested change

compressionLevel *int

compressionLevel *int // only used if compressionFormat is set

mtrmac · 2023-07-21T19:47:14Z

Thanks again!

flouthoc mentioned this pull request Jul 21, 2023

copy: implement instanceCopyClone for zstd compression #1987

Merged

mtrmac reviewed Jul 21, 2023

View reviewed changes

flouthoc force-pushed the fix-args-copysingleimage branch from 35af6a9 to bd894ce Compare July 21, 2023 18:42

flouthoc requested a review from mtrmac July 21, 2023 18:44

flouthoc force-pushed the fix-args-copysingleimage branch from bd894ce to 8d456e6 Compare July 21, 2023 18:45

mtrmac reviewed Jul 21, 2023

View reviewed changes

copy/single.go Outdated Show resolved Hide resolved

flouthoc force-pushed the fix-args-copysingleimage branch from 8d456e6 to 067a67d Compare July 21, 2023 19:03

flouthoc requested a review from mtrmac July 21, 2023 19:04

mtrmac reviewed Jul 21, 2023

View reviewed changes

copy/copy.go Outdated Show resolved Hide resolved

mtrmac reviewed Jul 21, 2023

View reviewed changes

copy/single.go Outdated Show resolved Hide resolved

flouthoc added 3 commits July 22, 2023 00:53

copy/single: wrap arguments in copySingleImageOptions

7983ec2

Create a wrapper around arguments of `copySingleImage` Signed-off-by: Aditya R <arajan@redhat.com>

copy/single: honor c.options.DestCtx for regular copy

adbfd0d

If its a regular copy callers might not set `compressionFormat` and `compressionLevel` in such case still honor compression which is set in DestinationCtx from copier. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the fix-args-copysingleimage branch from 067a67d to adbfd0d Compare July 21, 2023 19:23

flouthoc requested a review from mtrmac July 21, 2023 19:24

mtrmac approved these changes Jul 21, 2023

View reviewed changes

mtrmac reviewed Jul 21, 2023

View reviewed changes

mtrmac merged commit 30c87d4 into containers:main Jul 21, 2023
9 checks passed

flouthoc mentioned this pull request Aug 4, 2023

[release-5.26] backport API changes copy and list components from upstream. #2071

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copy/single: accept custom `*Options` and wrap arguments in `copySingleImageOptions` #2050

copy/single: accept custom `*Options` and wrap arguments in `copySingleImageOptions` #2050

flouthoc commented Jul 21, 2023

flouthoc commented Jul 21, 2023

mtrmac Jul 21, 2023

mtrmac Jul 21, 2023

flouthoc Jul 21, 2023

flouthoc Jul 21, 2023 •

edited

Loading

flouthoc Jul 21, 2023

mtrmac Jul 21, 2023

flouthoc Jul 21, 2023

mtrmac Jul 21, 2023

flouthoc commented Jul 21, 2023

mtrmac left a comment

mtrmac Jul 21, 2023

mtrmac Jul 21, 2023

mtrmac commented Jul 21, 2023

	compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the user explicitly requested one, or nil.
	compressionFormat *compressiontypes.Algorithm // Compression algorithm to use, if the caller explicitly requested one, or nil.

	compressionLevel *int
	compressionLevel *int // only used if compressionFormat is set

copy/single: accept custom *Options and wrap arguments in copySingleImageOptions #2050

copy/single: accept custom *Options and wrap arguments in copySingleImageOptions #2050

Conversation

flouthoc commented Jul 21, 2023

flouthoc commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc commented Jul 21, 2023

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Jul 21, 2023

copy/single: accept custom `*Options` and wrap arguments in `copySingleImageOptions` #2050

copy/single: accept custom `*Options` and wrap arguments in `copySingleImageOptions` #2050

flouthoc Jul 21, 2023 •

edited

Loading