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.

Sign up for GitHub

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

Jump to bottom

Cp cmd #643

Closed
wants to merge 1 commit into from
Closed

Cp cmd #643

wants to merge 1 commit into from

Conversation

fahedouch
Copy link
Member

@fahedouch fahedouch commented Dec 20, 2021 •
edited
Loading

nerdctl cp design #212

PS: only running container is support for the moment. I will implement not running container in other PR.

@fahedouch fahedouch marked this pull request as draft December 20, 2021 21:55
@fahedouch fahedouch changed the title Cp cmd [WIP] Cp cmd Dec 20, 2021
AkihiroSuda
AkihiroSuda reviewed Dec 21, 2021
View reviewed changes
cmd/nerdctl/cp.go Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda reviewed Dec 21, 2021
View reviewed changes
cmd/nerdctl/cp.go Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda linked an issue Dec 21, 2021 that may be closed by this pull request
nerdctl cp design #212
Closed
@fahedouch fahedouch self-assigned this Jan 7, 2022
@fahedouch fahedouch force-pushed the cp-cmd branch 2 times, most recently from b4d9989 to bb91977 Compare January 7, 2022 18:57
@fahedouch fahedouch marked this pull request as ready for review January 7, 2022 18:58
@fahedouch fahedouch force-pushed the cp-cmd branch 2 times, most recently from 13b63ee to 573c905 Compare January 14, 2022 16:24
@fahedouch fahedouch changed the title [WIP] Cp cmd Cp cmd Jan 14, 2022
@fahedouch fahedouch force-pushed the cp-cmd branch 8 times, most recently from e717ce6 to 61e0efe Compare January 16, 2022 12:56
@fahedouch
Copy link
Member Author

@AkihiroSuda @ktock would you plz review this PR.

I have some troubles to pass test TestCopyToContainer :

test target: "nerdctl"
=== RUN   TestCopyToContainer
    cp_test.go:42: copying /tmp/nerdctl-cp-test749964100/workFile from host to nerdctl-testcopytocontainer:/tmp
    cp_test.go:46: assertion failed: 0 (int) != 1 (res.ExitCode int): cat: can't open '/tmp/workFile': No such file or directory
        time="2022-01-16T12:59:46Z" level=fatal msg="exec failed with exit code 1"
        
--- FAIL: TestCopyToContainer (4.15s)

but thus works in my local environnement

@fahedouch fahedouch force-pushed the cp-cmd branch 3 times, most recently from 9b1bc6e to 5278782 Compare January 16, 2022 13:27
@fahedouch fahedouch force-pushed the cp-cmd branch 6 times, most recently from a814a73 to 3283d80 Compare March 25, 2022 22:12
@fahedouch
Copy link
Member Author

error obtaining VCS status: exec: "git": executable file not found in %PATH%

Needs rebase to pass CI

@AkihiroSuda fixed

@AkihiroSuda AkihiroSuda added this to the v0.18.1 (tentative) milestone Mar 26, 2022
@AkihiroSuda
Copy link
Member 

--- FAIL: TestCopyToContainer (24.51s)
=== RUN   TestCopyFromContainer
    cp_test.go:64: copying /etc/os-release from nerdctl-testcopyfromcontainer to C:\Windows\system32\config\systemprofile\AppData\Local\Temp\nerdctl-cp-test4134092264\os-release in the host
    cp_test.go:69: assertion failed: error is not nil: open C:\Windows\system32\config\systemprofile\AppData\Local\Temp\nerdctl-cp-test4134092264\os-release: The system cannot find the file specified.
--- FAIL: TestCopyFromContainer (1.56s)

https://cirrus-ci.com/task/6431751521173504?logs=build#L352

(You can just disable cp for Windows)

@fahedouch fahedouch force-pushed the cp-cmd branch 4 times, most recently from fb95b26 to a91732f Compare March 29, 2022 09:36
@fahedouch
Copy link
Member Author

cc @AkihiroSuda

AkihiroSuda
AkihiroSuda reviewed Apr 5, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda reviewed Apr 5, 2022
View reviewed changes
cmd/nerdctl/cp.go Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda reviewed Apr 5, 2022
View reviewed changes
cmd/nerdctl/cp.go
}

// recursiveTar makes a tar recursively
func recursiveTar(srcDir, srcBase, dstDir, dstBase string, user runtimespec.User, tw *tar.Writer, archive bool) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to mix up the tar command and Go's archive/tar?

Copy link
Member

@AkihiroSuda AkihiroSuda Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just using tar command on the host would be simpler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda I need archive/tar because I writing to io.PipeWriter here https://github.com/containerd/nerdctl/pull/643/files#diff-dc36d8c99566d6fecdc69b09e1781108cb3026a4a5dbbfe31e1df4b4d2572c3bR331 . I could have used archive/tar everywhere but it can be a possible evolution (future PR) to avoid a vulnerable binary tar

@fahedouch fahedouch force-pushed the cp-cmd branch 3 times, most recently from e10cd7e to 05b1840 Compare April 11, 2022 09:30
@fahedouch
cp cmd
db97e2f 
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
AkihiroSuda
AkihiroSuda reviewed Apr 18, 2022
View reviewed changes
pkg/copyutil/copyutil.go
// So we first replace each slash ('/') character in path with a separator character
// then we clean the path.
// A trailing "." is preserved if the original path specified the current directory.
func SplitPathDirEntry(path string) (dir, base string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/search?q=SplitPathDirEntry&type=code

When you copied codes from somewhere else, please confirm the origin and the license.

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 18, 2022 •
edited
Loading

Docker:

$ docker run -d --name foo alpine sleep infinity
$ docker cp .bashrc foo:/foo
$ docker exec foo ls -l /foo
-rw-r--r--    1 1001     1001          4209 Feb  6 08:49 /foo

This PR:

$ nerdctl run -d --name foo alpine sleep infinity
$ nerdctl cp .bashrc foo:/foo
FATA[0000] /foo should exists
$ nerdctl cp .bashrc foo:/
$ nerdctl exec foo ls -l /.bashrc 
-rw-r--r--    1 root     root          4209 Apr 18 08:24 /.bashrc

nerdctl cp .bashrc foo:/foo didn't work.
nerdctl cp .bashrc foo:/ worked but the permission and the timestamp doesn't match Docker.

@AkihiroSuda AkihiroSuda mentioned this pull request Apr 18, 2022
Merged
@AkihiroSuda
Copy link
Member

Carry PR:

@AkihiroSuda AkihiroSuda removed this from the v0.19.0 (tentative) milestone Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@ktock ktock Awaiting requested review from ktock

Assignees

@fahedouch fahedouch

Labels
enhancement New feature or request impact/major status/needs-design-discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nerdctl cp design
2 participants
@fahedouch @AkihiroSuda