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,refactor: split copyOneImage and copyMultipleImages #1877

Closed
wants to merge 1 commit into from

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Mar 8, 2023

As discussed in #1875 it is suggested to keep the two implementation in different files since there independent complexity is increasing.

As discussed containers#1875 it is
suggested to keep the two implementation in different files since there
independent complexity is increasing.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
Copy link
Contributor Author

flouthoc commented Mar 8, 2023

@mtrmac PTAL, the only caller of copyOneImage and copyMultipleImage is func Image( which i kept intact but we can also move parts of it to single and multiple.

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!

Please also move

  • Smaller utility functions only used in the moved functions (e.g. compareImageDestinationManifestEqual). In particular all of the layer code that still is in copy.go should move to single.go
  • Similarly, the global constants/variables that only are used in the moved functions (e.g. compressionBufferSize), to the same files
  • imageCopier itself, to single.go (and probably all methods on imageCopier).
  • Unit tests should move with the tested functions.

(If you’d like, I can prepare a PR for these moves — the choice basically depends on how it would be more convenient for you to rebase/update the other outstanding work.)

@flouthoc
Copy link
Contributor Author

flouthoc commented Mar 9, 2023

(If you’d like, I can prepare a PR for these moves — the choice basically depends on how it would be more convenient for you to rebase/update the other outstanding work.)

@mtrmac Sure sounds good to me, I'll rebase on top of these changes.

@mtrmac
Copy link
Collaborator

mtrmac commented Mar 9, 2023

(If you’d like, I can prepare a PR for these moves

#1878

@mtrmac
Copy link
Collaborator

mtrmac commented Mar 14, 2023

#1878 was merged.

@mtrmac mtrmac closed this Mar 14, 2023
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.

2 participants