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

copy: add support for ForceCompressionFormat #2068

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Aug 2, 2023

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:

@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 2, 2023

@mtrmac @vrothberg PTAL

Copy link
Collaborator

@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 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
@@ -230,6 +230,16 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
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}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, added test.

@flouthoc flouthoc force-pushed the implement-forcecompressionformat branch from 90d4dd5 to b0768e3 Compare August 2, 2023 04:22
@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 2, 2023

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

@vrothberg @mtrmac PTAL

@flouthoc flouthoc requested a review from mtrmac August 2, 2023 04:24
@flouthoc flouthoc force-pushed the implement-forcecompressionformat branch 2 times, most recently from 587ca74 to 76692b3 Compare August 2, 2023 05:36
copy/multiple.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the implement-forcecompressionformat branch 2 times, most recently from d357f4e to a7556dd Compare August 2, 2023 07:45
Copy link
Collaborator

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

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 flouthoc force-pushed the implement-forcecompressionformat branch from a7556dd to 12c3048 Compare August 3, 2023 06:08
@flouthoc flouthoc requested a review from mtrmac August 3, 2023 14:53
Copy link
Collaborator

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

@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

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 <[email protected]>
@mtrmac mtrmac force-pushed the implement-forcecompressionformat branch from 12c3048 to f406c36 Compare August 3, 2023 18:12
@mtrmac mtrmac merged commit 9446ffa into containers:main Aug 3, 2023
9 checks passed
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 8, 2023
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 8, 2023
Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 8, 2023
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 8, 2023
Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 8, 2023
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 8, 2023
Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 9, 2023
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 9, 2023
Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 9, 2023
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 9, 2023
Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 11, 2023
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 11, 2023
Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 11, 2023
flouthoc added a commit to flouthoc/common that referenced this pull request Aug 11, 2023
Implement containers/image#2068 for
libimage/manifest.Push

Signed-off-by: Aditya R <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants