sys: move ForceRemoveAll to integration/client#7335
Conversation
|
Failures look unrelated (cri-tools tests look unhappy on other PR's as well). But, it did make me head to that repository to see if it might be using this function (wasn't the case, but I found some deprecated use of a docker package, so 🤷♂️ 😂 kubernetes-sigs/cri-tools#969) |
|
Ah! Needs a rebase now; let me do so in a minute |
ForceRemoveAll was only used in tests/fuzzing, but added hcsshim as dependency for the sys package. Moving this to integration/client makes the "sys" package slightly more lightweight, and may help simplifying dependency-management. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
move_sys_forceremoveall
branch
from
bc35681 to
2e677c9
Compare
|
done 👍 |
| // forceRemoveAll is the same as os.RemoveAll, but is aware of io.containerd.snapshotter.v1.windows | ||
| // and uses hcsshim to unmount and delete container layers contained therein, in the correct order, | ||
| // when passed a containerd root data directory (i.e. the `--root` directory for containerd). | ||
| func forceRemoveAll(path string) error { |
There was a problem hiding this comment.
FWIW; if we would ever need this utility outside of these integration tests, I would suggest to then move it inside a package that deals with these snapshots (perhaps to a utils package or utils.go file in https://github.com/containerd/containerd/tree/v1.6.8/snapshots/windows). That way we can import it more "focussed" where needed.
There was a problem hiding this comment.
I'm in agreeance with this; sorry I missed this change. It seems extremely snapshotter focused and not very general purpose so +1 from me
There was a problem hiding this comment.
Thanks for looking! Yes, so currently these utilities are only used in the integration tests, so I didn't want to give them a larger "audience" than needed by moving them into the snapshots/windows package. My comment was mostly for IF there would be a demand for that.
And countering myself; we should be careful with creating a utils package (or at least consider if we want to); naming a package utils is commonly discouraged, as it's too easy to become a kitchen-sink of "I don't know where to put it, just dump it in "utils". And before we know if, we created a new "sys" package with too many dependencies that's imported in too many places 😂
There was a problem hiding this comment.
Oh double comment after the fact on an already merged PR.. I was actually referring to agreeing with the entire change not the utils comment specifically LOL I realized I'm commenting on a specific comment here
5 participants
ForceRemoveAll was only used in tests/fuzzing, but added hcsshim as dependency
for the sys package. Moving this to integration/client makes the "sys" package
slightly more lightweight, and may help simplifying dependency-management.