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.

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

Refactor data passing in c/image/copy #2048

Merged
merged 23 commits into from
Jul 19, 2023
Merged

Refactor data passing in c/image/copy #2048

merged 23 commits into from
Jul 19, 2023

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jul 19, 2023

This is motivated by the concurrent Zstd work, to make it more maintainable to pass data from and to copySingleImage.

  • Add copier.options, copier.policyContext and copier.unparsedToplevel instead of passing them around in parameters
  • Eliminate fields of copier and imageCopier that are now reachable through .options
  • Return a copySingleImageResult from copySingleImage

@flouthoc I realize this is disruptive, and I want to do whatever necessary not to disrupt or slow down your work. Let me know what works for you — feel free to completely ignore this (and I’ll rebase this later as necessary), to reimplement parts yourself in some other way, or to tell me to split this up (maybe only the copySingleImageResult part?).

Others: Please don’t merge this without an ACK from @flouthoc .

mtrmac added 23 commits July 19, 2023 01:20
@mtrmac
Remove a comment
3830771 
- Generally I discourage unsing named return values, it's easy
  to miss that it wasn't set
- I have no idea what the "in the middle of a multi-streamed copy"
  paragraph refers to.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Add *copy.Options to copier
b918ffb 
... so that we don't have to carry it around in extra
parameters. Migrating individual functions will follow.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use copier.options in setupSigners
5452350 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use copier.options in sourceSignatures
c31be89 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Make compareImageDestinationManifestEqual a method on imageCopier
11d808e 
... so that we can eliminate three parameters.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use copier.options in copySingleImage
8ec8e39 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use copier.options in copyMultipleImages
c1057fd 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Consistently use c.options in copy.Image after it is set
5b50977 
... so that the code looks the same here and in possible callees.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Eliminate copier.progress and progressInterval
1c32cfe 
Use options.Progress and ProgressInterval directly.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Eliminate copier.ociDecryptConfig
e75f36a 
Use c.options.OciDecryptConfig directly.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Eliminate copier.ociEncryptConfig
412eb03 
Use c.options.OciEncryptConfig directly.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Eliminate copier.downloadForeignLayers
5664d58 
Use c.options.DownloadForeignLayers directly.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Eliminate imageCopier.ociEncryptLayers
660d779 
Use ic.c.options.OciEncryptLayers directly.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Introduce copier.unparsedToplevel
cb87cf6 
So that we don't need to pass it around in a parameter.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use copier.unparsedToplevel in copySingleImage
3710961 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use c.unparsedToplevel in copyMultipleImages
10de278 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Eliminate the unparsedToplevel variable in copy.Image
1492784 
Use copier.unparsedToplevel now that it exists.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Add policyContext to copier
3d065ad 
... so that we don't need to carry it around in parameters.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use copier.policyContext in copySingleImage
906b1d3 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Remove the policyContext parameter from copyMultipleImages
6b78a77 
It is no longer used.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Return a new struct copySingleImageResult from copySingleImage
c72e8a6 
... so that we don't have so many unnamed return values, and
we can manage the return values as a batch.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Have compareImageDestinationManifestEqual return a *copySingleImageRe…
15bbf66 
…sult on a match

... so that the caller doesn't have to assemble it. Using a pointer-or-nil eliminates
a separate boolean.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Track the manifest upload data in a wipResult variable
e43b64b 
... instead of three separate ones.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac mtrmac mentioned this pull request Jul 19, 2023
Merged
@flouthoc
Copy link
Contributor

flouthoc commented Jul 19, 2023 •
edited
Loading

This LGTM I'll rebase my PR on top of this. I don't have merge access.

@flouthoc
Copy link
Contributor

@vrothberg Could you PTAL

vrothberg
vrothberg approved these changes Jul 19, 2023
View reviewed changes
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.

Very nice refactor and with surgical precision :)

LGTM

@vrothberg vrothberg merged commit b739592 into containers:main Jul 19, 2023
9 checks passed
@mtrmac mtrmac deleted the copy-single-api branch July 19, 2023 18:25
@flouthoc flouthoc mentioned this pull request Jul 21, 2023
Merged
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 21, 2023
@flouthoc
copy/single: add custom compressionFormat, compressionLevel fields
bd894ce 
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>
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 21, 2023
@flouthoc
copy/single: add custom compressionFormat, compressionLevel fields
8d456e6 
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>
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 21, 2023
@flouthoc
copy/single: add custom compressionFormat, compressionLevel fields
fdff65a 
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>
flouthoc added a commit to flouthoc/image that referenced this pull request Jul 21, 2023
@flouthoc
copy/single: add custom compressionFormat, compressionLevel fields
481ed6a 
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>
flouthoc added a commit to flouthoc/image that referenced this pull request Aug 4, 2023
@flouthoc
copy/single: add custom compressionFormat, compressionLevel fields
6272732 
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>
@flouthoc flouthoc mentioned this pull request Aug 4, 2023
Closed
mtrmac pushed a commit to mtrmac/image that referenced this pull request Aug 4, 2023
@flouthoc @mtrmac
copy/single: add custom compressionFormat, compressionLevel fields
b59d47c 
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>
mtrmac pushed a commit to mtrmac/image that referenced this pull request Aug 5, 2023
@flouthoc @mtrmac
copy/single: add custom compressionFormat, compressionLevel fields
2a961fd 
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>
@mtrmac mtrmac mentioned this pull request Oct 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vrothberg vrothberg vrothberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mtrmac @flouthoc @vrothberg