Narrow dependencies of pkg/system#39873
Conversation
|
I'm not familiar with this repo's CI -- is this failure possibly related to my PR or expected flakiness? |
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
Thanks for submitting this.
What's the driver for this PR? (no pun intended)
There was a problem hiding this comment.
This is an exported function, we shouldn't have an argument that is an unexported type.
Maybe something like PathVerifier? (I hate naming things).
CheckSystemDriveAndRemoveDriveLetter depends on pathdriver.PathDriver unnecessarily. This depends on the minimal interface that it actually needs, to avoid callers from unnecessarily bringing in a containerd/continuity dependency. Signed-off-by: Jon Johnson <[email protected]>
ebd0b5d to
57ade26
Compare
I tried to pull in some config file handling code from https://godoc.org/github.com/docker/cli/cli/config and was a bit surprised by how many packages I ended up needing to import. See here. I figured I'd start at the leaves and work my way up from there. The root issue is actually the github.com/docker/docker/pkg/homedir dependency, but given this comment I'm not actually confident that it's possible to fix the underlying issue. If you have any better ideas for re-organizing packages to avoid importing logrus, etc., I'm open to it.
😅 |
|
Probably docker/cli should make their own package. The usage there is very simple but the dependencies it creates are huge. |
|
@cpuguy83 PTAL |
Seems reasonable -- I'll see how hard it would be to inline their homedir usage. I think this PR is still reasonable to merge, since it helps untangle things a bit, but feel free to close it if you disagree. |
dmcgowan
left a comment
dmcgowan
left a comment
There was a problem hiding this comment.
LGTM
Note we can start getting rid of github.com/containerd/continuity/pathdriver completely by deprecating the LCOW build support.
6 participants
CheckSystemDriveAndRemoveDriveLetter depends on pathdriver.PathDriver
unnecessarily. This depends on the minimal interface that it actually
needs, to avoid callers from unnecessarily bringing in a
containerd/continuity dependency.
Signed-off-by: Jon Johnson [email protected]
- What I did
Reduce dependencies of pkg/system/path.go
- How I did it
Define an interface locally instead of using the interface from an external package.
- How to verify it
No behavior should change.
- A picture of a cute animal (not mandatory but encouraged)
My puggle, Grace Anne: