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

Remove github.com/otiai10/copy dependency #2239

Closed
wants to merge 3 commits into from
Closed

Remove github.com/otiai10/copy dependency #2239

wants to merge 3 commits into from

Conversation

alexandear
Copy link

This PR replaces usage of copy.Copy with hand-written copyDir function. It's not worth keeping this dependency used only in tests.

Yeah, copyDir is not ideal, and copy.Copy is much more powerful and covers many of OS's corner cases, but for tests it's good. In the future, we can return this dependency if copy.Copy will be needed for the prod code.

@rhatdan
Copy link
Member

rhatdan commented Jan 8, 2024

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

I don’t feel strongly about the change itself.

It’s just… This is a unit test. It does not show up in any dependency of c/image. So why is it important to trim dependencies? To me, adding and reviewing extra ~55 lines of code for no end-user benefit seems like a bad trade. So what am I missing? Are there any negative effects of that dependency I don’t know about, which would also apply to other dependencies in the future?

oci/layout/oci_delete_test.go Outdated Show resolved Hide resolved
@alexandear
Copy link
Author

@mtrmac The README indeed categorizes containers/image as "image is a set of Go libraries", implying its main purpose is to be imported by users. Each time a user imports the package, it automatically imports its dependencies. This reduces download speed and increases the user's application dependency list. To maintain efficient use and management, it's critical to keep a minimal list of dependencies.

Adding to this, having fewer dependencies enhances the overall control we have on the application. Reviewing and adding 55 lines of code is a one-time task. But every extra dependency is an added layer of complexity, potential security risk, and another component that should be maintained and kept updated. We are simplifying the application maintenance process by trimming down dependencies, just as Rob Pike mentioned about keeping the dependency tree small (check video).

https://medium.com/@cep21/aspects-of-a-good-go-library-7082beabb403

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 9, 2024

Each time a user imports the package, it automatically imports its dependencies.

I mean…

% cat foo.go 
package foo

import "github.com/containers/image/v5/oci/layout"

func main() {
  _ = layout.Transport
}
% go mod tidy && go mod vendor
% grep -lr 10/copy
./go.sum

Test-only dependencies don’t really “infect” the consumer.

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.

The recent commits need to follow https://github.com/containers/image/blob/main/CONTRIBUTING.md#sign-your-prs .

(And eventually, please squash them into one.)

@alexandear
Copy link
Author

Actually, you're right. Test dependencies won't inflect customer since Go 1.18. Found this change
https://go-review.googlesource.com/c/go/+/357310/

Closing PR.

@alexandear alexandear closed this Jan 9, 2024
@alexandear alexandear deleted the remove-otiai10-copy-dependency branch January 9, 2024 19:13
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