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.

Sign up for GitHub

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

Jump to bottom

copy: add support for `ForceCompressionFormat` #2068

Merged

mtrmac merged 1 commit into containers:main from flouthoc:implement-forcecompressionformat

Aug 3, 2023

Contributor

flouthoc commented Aug 2, 2023 •

edited

Loading

ForceCompressionFormat allows end users to force selected compression format (set in DestinationCtx.CompressionFormat) which ensures that while copying blobs of other compression algorithms are not reused.

Following flag is a frontend wrapper for: #2023

Will help in:

Contributor Author

flouthoc commented Aug 2, 2023

@mtrmac @vrothberg PTAL

mtrmac reviewed

View reviewed changes

Collaborator

mtrmac left a comment

Thanks for working on this!

I think the two non-copyMultipleImages paths should also implement this (or, at the very least, fail with an error). It seems to me that the three locations would benefit from a small shared helper function.

copy/copy.go Outdated Show resolved Hide resolved

copy/copy.go Outdated Show resolved Hide resolved

copy/multiple.go Outdated Show resolved Hide resolved

copy/multiple.go Outdated

               			logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList))
               			c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList))
               			unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest)
+              			singleImageOpts := copySingleImageOptions{requireCompressionFormatMatch: false}

Collaborator

mtrmac Aug 2, 2023

This value is not actually used AFAICS

I would also prefer it to be set in prepare…; that way we can unit-test this. (I’m not saying that a unit test is required.)

Contributor Author

flouthoc Aug 2, 2023

done, added test.

flouthoc force-pushed the implement-forcecompressionformat branch from 90d4dd5 to b0768e3 Compare

August 2, 2023 04:22

Contributor Author

flouthoc commented Aug 2, 2023 •

edited

Loading

Addressed all comments ~~but one doubt which needs to be discussed here: #2068 (comment)~~

@vrothberg @mtrmac PTAL

flouthoc requested a review from mtrmac

August 2, 2023 04:24

flouthoc force-pushed the implement-forcecompressionformat branch 2 times, most recently from 587ca74 to 76692b3 Compare

August 2, 2023 05:36

vrothberg approved these changes

View reviewed changes

copy/multiple.go Outdated Show resolved Hide resolved

flouthoc force-pushed the implement-forcecompressionformat branch 2 times, most recently from d357f4e to a7556dd Compare

August 2, 2023 07:45

mtrmac reviewed

View reviewed changes

Collaborator

mtrmac left a comment

ACK to the logic, now just some cleanup, please.

copy/multiple.go Outdated Show resolved Hide resolved

copy/copy.go Outdated Show resolved Hide resolved

copy/copy.go Outdated Show resolved Hide resolved

flouthoc force-pushed the implement-forcecompressionformat branch from a7556dd to 12c3048 Compare

August 3, 2023 06:08

flouthoc requested a review from mtrmac

August 3, 2023 14:53

mtrmac approved these changes

View reviewed changes

Collaborator

mtrmac left a comment

Thanks! LGTM.

copy/copy.go

@@ @@ -163,6 +167,14 @@ type copier struct { @@
               	signersToClose                []*signer.Signer    // Signers that should be closed when this copier is destroyed.
               }
+              // Internal function to validate `requireCompressionFormatMatch` for copySingleImageOptions
+              func shouldRequireCompressionFormatMatch(options *Options) (bool, error) {

Collaborator

mtrmac Aug 3, 2023

(Hiding this somewhere at the bottom, not near the top of the file, might help newcomers to the codebase who want to understand what this file is about.)


          copy: add support for ForceCompressionFormat

f406c36

ForceCompressionFormat allows end users to force selected compression format
(set in DestinationCtx.CompressionFormat) which ensures that while copying
blobs of other compression algorithms are not reused.

Following flag is a frontend wrapper for: containers#2023
Will help in:
* containers/buildah#4613
* containers/podman#18660

Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac force-pushed the implement-forcecompressionformat branch from 12c3048 to f406c36 Compare

August 3, 2023 18:12

mtrmac merged commit 9446ffa into containers:main

flouthoc added a commit to flouthoc/common that referenced this pull request


          libimage/copier: wire ForceCompressionFormat for image copy

6da326e

Implement containers/image#2068 for
libimage/copier.

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          manifest: add support ForceCompressionFormat

02bf3bc

Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc mentioned this pull request

libimage: add support for ForceCompressionFormat for image copier and manifest containers/common#1602

Merged

flouthoc added a commit to flouthoc/common that referenced this pull request


          libimage/copier: wire ForceCompressionFormat for image copy

2a1705a

Implement containers/image#2068 for
libimage/copier.

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          manifest: add support ForceCompressionFormat

d94715a

Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          libimage/copier: wire ForceCompressionFormat for image copy

a1478d1

Implement containers/image#2068 for
libimage/copier.

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          manifest: add support ForceCompressionFormat

170f138

Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          libimage/copier: wire ForceCompressionFormat for image copy

cfafdfb

Implement containers/image#2068 for
libimage/copier.

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          manifest: add support ForceCompressionFormat

6cf031c

Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          libimage/copier: wire ForceCompressionFormat for image copy

45ca978

Implement containers/image#2068 for
libimage/copier.

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          manifest: add support ForceCompressionFormat

a045159

Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          libimage/copier: wire ForceCompressionFormat for image copy

ce1afe5

Implement containers/image#2068 for
libimage/copier.

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          manifest: add support ForceCompressionFormat

58ce959

Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          libimage/copier: wire ForceCompressionFormat for image copy

acd9733

Implement containers/image#2068 for
libimage/copier.

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc added a commit to flouthoc/common that referenced this pull request


          manifest: add support ForceCompressionFormat

248cc73

Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac mentioned this pull request

docker/internal/tarfile has incorrect media type in manifests; copy layer format is not manifest-format dependent #1270

Open

mtrmac mentioned this pull request

Expose copy.Options.ForceCompressionFormat containers/skopeo#2343

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet