feat: add support for container cp with tarballs #4704
Conversation
| - `nerdctl cp [OPTIONS] CONTAINER:SRC_PATH DEST_PATH|-` | ||
| - `nerdctl cp [OPTIONS] SRC_PATH|- CONTAINER:DEST_PATH` | ||
|
|
||
| Using `-` as the `SRC_PATH` streams the contents of `STDIN` as a tar archive. The command extracts the content of the tar to the `DEST_PATH` in container's filesystem. In this case, `DEST_PATH` must specify a directory. Using `-` as the `DEST_PATH` streams the contents of the resource as a tar archive to `STDOUT`. |
There was a problem hiding this comment.
FYI, copied this exact wording from https://docs.docker.com/reference/cli/docker/container/cp/#corner-cases. LMK if this is a problem in any way.
|
Will get to adding testing tomorrow. Having some trouble adding tests to the existing testing suite for now. |
sondavidb
force-pushed
the
add-container-cp-tarballs
branch
from
5bd5c65 to
91f3775
Compare
|
I was very optimistic about the testing suite, stuffing the test cases in there took a lot of effort. This suite could certainly split up some functions (the amount of if statements in here is hard to keep track of, to say the least...) In any case, tests passed locally so hoping this should be good to go 🙂 |
sondavidb
force-pushed
the
add-container-cp-tarballs
branch
from
91f3775 to
8fe1537
Compare
|
Test failures look unrelated to changes? |
| @@ -134,6 +142,14 @@ func getPathSpecFromHost(originalPath string) (*pathSpecifier, error) { | |||
|
|
|||
| // getPathSpecFromHost builds a pathSpecifier from a container location | |||
There was a problem hiding this comment.
not related but can we change this to getPathSpecFromContainer
pkg/containerutil/cp_linux.go
Outdated
| if options.Container2Host && isGNUTar { | ||
| tarX = append(tarX, "--no-same-owner") | ||
| var tarX []string | ||
| if destinationSpec.originalPath == "-" { |
There was a problem hiding this comment.
nit: can we add this to the containerCP option somthing like option.Stdin and option.Stdout and use that as the source of truth instead of doing this comparison for clarity.
There was a problem hiding this comment.
Sure, added options FromStdin and ToStdout in ContainerCpOptions + added fromStdin and toStdout in pathSpecifier
|
|
||
| // getPathSpecFromHost builds a pathSpecifier from a container location | ||
| func getPathSpecFromContainer(originalPath string, conSpec *oci.Spec, containerHostRoot string) (*pathSpecifier, error) { | ||
| if originalPath == "-" { |
There was a problem hiding this comment.
this condition here is not intuitve, can we skip calling them when we have stdin and stdout options are true for their respective validations.
| // getPathSpecFromHost builds a pathSpecifier from a host location | ||
| // errors with errDoesNotExist, errIsNotADir, "EvalSymlinks: too many links", or other hard filesystem errors from lstat/stat | ||
| func getPathSpecFromHost(originalPath string) (*pathSpecifier, error) { | ||
| if originalPath == "-" { |
There was a problem hiding this comment.
same response as below as this case is unintuitive especially for isADir case.
| description: "DEST_PATH is tarball", | ||
| destinationSpec: "-", | ||
| expect: icmd.Expected{ | ||
| ExitCode: 1, |
There was a problem hiding this comment.
need error type, otherwise test is not clear why it should fail. may be also adding a bit of description on the error would help
| }, | ||
| }, | ||
| { | ||
| description: "DEST_PATH is a tarball", |
There was a problem hiding this comment.
all description are DEST_PATH is a tarball? not sure i understand this test. my understanding is we copy from container into stdout if we have "-" in destination spec.
There was a problem hiding this comment.
Yeah, changed to DEST_PATH is stdout. I used tarball since that was the format of the output but what you said is more correct I think. Made changes across testing suite for this.
Signed-off-by: David Son <[email protected]>
sondavidb
force-pushed
the
add-container-cp-tarballs
branch
from
8fe1537 to
97facf2
Compare
2 participants
Closes #4691
This PR should allow support for the following in
container cp:-in src to allow streaming the contents of a tarball into a container.-in dst to allow streaming the contents of a container into a tarball that is written to stdout