From c70b6c9f351fdc737d5adb5626c58fc2e2fe05d7 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Tue, 27 Feb 2018 13:42:29 +0100 Subject: [PATCH 1/4] Clean some docker_cli_build_tests that are cli-only Remove TestBuildRenamedDockerfile and TestBuildDockerfileOutsideContext that are cli-only tests (and already tested in the docker/cli repository). Also adds some comments on few tests that could be migrate to docker/cli. Signed-off-by: Vincent Demeester (cherry picked from commit 894c213b3bd6f4d8f344837b5b5084360a013680) Signed-off-by: Vincent Demeester --- .../integration-cli/docker_cli_build_test.go | 134 +++--------------- 1 file changed, 18 insertions(+), 116 deletions(-) diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go index acd44b6a93..335aaed706 100644 --- a/components/engine/integration-cli/docker_cli_build_test.go +++ b/components/engine/integration-cli/docker_cli_build_test.go @@ -1439,6 +1439,7 @@ func (s *DockerSuite) TestBuildRelativeCopy(c *check.C) { )) } +// FIXME(vdemeester) should be unit test func (s *DockerSuite) TestBuildBlankName(c *check.C) { name := "testbuildblankname" testCases := []struct { @@ -2066,6 +2067,7 @@ func (s *DockerSuite) TestBuildNoContext(c *check.C) { } } +// FIXME(vdemeester) migrate to docker/cli e2e func (s *DockerSuite) TestBuildDockerfileStdin(c *check.C) { name := "stdindockerfile" tmpDir, err := ioutil.TempDir("", "fake-context") @@ -2085,6 +2087,7 @@ CMD ["cat", "/foo"]`), c.Assert(strings.TrimSpace(string(res)), checker.Equals, `[cat /foo]`) } +// FIXME(vdemeester) migrate to docker/cli tests (unit or e2e) func (s *DockerSuite) TestBuildDockerfileStdinConflict(c *check.C) { name := "stdindockerfiletarcontext" icmd.RunCmd(icmd.Cmd{ @@ -2401,6 +2404,7 @@ func (s *DockerSuite) TestBuildDockerignoringDockerfile(c *check.C) { build.WithFile("Dockerfile", dockerfile), build.WithFile(".dockerignore", "Dockerfile\n"), )) + // FIXME(vdemeester) why twice ? buildImageSuccessfully(c, name, build.WithBuildContext(c, build.WithFile("Dockerfile", dockerfile), build.WithFile(".dockerignore", "./Dockerfile\n"), @@ -2420,6 +2424,7 @@ func (s *DockerSuite) TestBuildDockerignoringRenamedDockerfile(c *check.C) { build.WithFile("MyDockerfile", dockerfile), build.WithFile(".dockerignore", "MyDockerfile\n"), )) + // FIXME(vdemeester) why twice ? buildImageSuccessfully(c, name, cli.WithFlags("-f", "MyDockerfile"), build.WithBuildContext(c, build.WithFile("Dockerfile", "Should not use me"), build.WithFile("MyDockerfile", dockerfile), @@ -3045,6 +3050,7 @@ func (s *DockerSuite) TestBuildAddTarXzGz(c *check.C) { buildImageSuccessfully(c, name, build.WithExternalBuildContext(ctx)) } +// FIXME(vdemeester) most of the from git tests could be moved to `docker/cli` e2e tests func (s *DockerSuite) TestBuildFromGit(c *check.C) { name := "testbuildfromgit" git := fakegit.New(c, "repo", map[string]string{ @@ -3422,6 +3428,7 @@ func (s *DockerSuite) TestBuildLabelsCache(c *check.C) { } +// FIXME(vdemeester) port to docker/cli e2e tests (api tests should test suppressOutput option though) func (s *DockerSuite) TestBuildNotVerboseSuccess(c *check.C) { // This test makes sure that -q works correctly when build is successful: // stdout has only the image ID (long image ID) and stderr is empty. @@ -3472,6 +3479,7 @@ func (s *DockerSuite) TestBuildNotVerboseSuccess(c *check.C) { } +// FIXME(vdemeester) migrate to docker/cli tests func (s *DockerSuite) TestBuildNotVerboseFailureWithNonExistImage(c *check.C) { // This test makes sure that -q works correctly when build fails by // comparing between the stderr output in quiet mode and in stdout @@ -3492,6 +3500,7 @@ func (s *DockerSuite) TestBuildNotVerboseFailureWithNonExistImage(c *check.C) { } } +// FIXME(vdemeester) migrate to docker/cli tests func (s *DockerSuite) TestBuildNotVerboseFailure(c *check.C) { // This test makes sure that -q works correctly when build fails by // comparing between the stderr output in quiet mode and in stdout @@ -3519,6 +3528,7 @@ func (s *DockerSuite) TestBuildNotVerboseFailure(c *check.C) { } } +// FIXME(vdemeester) migrate to docker/cli tests func (s *DockerSuite) TestBuildNotVerboseFailureRemote(c *check.C) { // This test ensures that when given a wrong URL, stderr in quiet mode and // stderr in verbose mode are identical. @@ -3548,6 +3558,7 @@ func (s *DockerSuite) TestBuildNotVerboseFailureRemote(c *check.C) { } } +// FIXME(vdemeester) migrate to docker/cli tests func (s *DockerSuite) TestBuildStderr(c *check.C) { // This test just makes sure that no non-error output goes // to stderr @@ -3688,67 +3699,6 @@ CMD cat /foo/file`), } -// FIXME(vdemeester) part of this should be unit test, other part should be clearer -func (s *DockerSuite) TestBuildRenamedDockerfile(c *check.C) { - ctx := fakecontext.New(c, "", fakecontext.WithFiles(map[string]string{ - "Dockerfile": "FROM busybox\nRUN echo from Dockerfile", - "files/Dockerfile": "FROM busybox\nRUN echo from files/Dockerfile", - "files/dFile": "FROM busybox\nRUN echo from files/dFile", - "dFile": "FROM busybox\nRUN echo from dFile", - "files/dFile2": "FROM busybox\nRUN echo from files/dFile2", - })) - defer ctx.Close() - - cli.Docker(cli.Args("build", "-t", "test1", "."), cli.InDir(ctx.Dir)).Assert(c, icmd.Expected{ - Out: "from Dockerfile", - }) - - cli.Docker(cli.Args("build", "-f", filepath.Join("files", "Dockerfile"), "-t", "test2", "."), cli.InDir(ctx.Dir)).Assert(c, icmd.Expected{ - Out: "from files/Dockerfile", - }) - - cli.Docker(cli.Args("build", fmt.Sprintf("--file=%s", filepath.Join("files", "dFile")), "-t", "test3", "."), cli.InDir(ctx.Dir)).Assert(c, icmd.Expected{ - Out: "from files/dFile", - }) - - cli.Docker(cli.Args("build", "--file=dFile", "-t", "test4", "."), cli.InDir(ctx.Dir)).Assert(c, icmd.Expected{ - Out: "from dFile", - }) - - dirWithNoDockerfile, err := ioutil.TempDir(os.TempDir(), "test5") - c.Assert(err, check.IsNil) - nonDockerfileFile := filepath.Join(dirWithNoDockerfile, "notDockerfile") - if _, err = os.Create(nonDockerfileFile); err != nil { - c.Fatal(err) - } - cli.Docker(cli.Args("build", fmt.Sprintf("--file=%s", nonDockerfileFile), "-t", "test5", "."), cli.InDir(ctx.Dir)).Assert(c, icmd.Expected{ - ExitCode: 1, - Err: fmt.Sprintf("unable to prepare context: the Dockerfile (%s) must be within the build context", nonDockerfileFile), - }) - - cli.Docker(cli.Args("build", "-f", filepath.Join("..", "Dockerfile"), "-t", "test6", ".."), cli.InDir(filepath.Join(ctx.Dir, "files"))).Assert(c, icmd.Expected{ - Out: "from Dockerfile", - }) - - cli.Docker(cli.Args("build", "-f", filepath.Join(ctx.Dir, "files", "Dockerfile"), "-t", "test7", ".."), cli.InDir(filepath.Join(ctx.Dir, "files"))).Assert(c, icmd.Expected{ - Out: "from files/Dockerfile", - }) - - cli.Docker(cli.Args("build", "-f", filepath.Join("..", "Dockerfile"), "-t", "test8", "."), cli.InDir(filepath.Join(ctx.Dir, "files"))).Assert(c, icmd.Expected{ - ExitCode: 1, - Err: "must be within the build context", - }) - - tmpDir := os.TempDir() - cli.Docker(cli.Args("build", "-t", "test9", ctx.Dir), cli.InDir(tmpDir)).Assert(c, icmd.Expected{ - Out: "from Dockerfile", - }) - - cli.Docker(cli.Args("build", "-f", "dFile2", "-t", "test10", "."), cli.InDir(filepath.Join(ctx.Dir, "files"))).Assert(c, icmd.Expected{ - Out: "from files/dFile2", - }) -} - func (s *DockerSuite) TestBuildFromMixedcaseDockerfile(c *check.C) { testRequires(c, UnixCli) // Dockerfile overwrites dockerfile on windows testRequires(c, DaemonIsLinux) @@ -3772,6 +3722,7 @@ func (s *DockerSuite) TestBuildFromMixedcaseDockerfile(c *check.C) { }) } +// FIXME(vdemeester) should migrate to docker/cli tests func (s *DockerSuite) TestBuildFromURLWithF(c *check.C) { server := fakestorage.New(c, "", fakecontext.WithFiles(map[string]string{"baz": `FROM busybox RUN echo from baz @@ -3798,6 +3749,7 @@ RUN find /tmp/`})) } +// FIXME(vdemeester) should migrate to docker/cli tests func (s *DockerSuite) TestBuildFromStdinWithF(c *check.C) { testRequires(c, DaemonIsLinux) // TODO Windows: This test is flaky; no idea why ctx := fakecontext.New(c, "", fakecontext.WithDockerfile(`FROM busybox @@ -3840,61 +3792,6 @@ func (s *DockerSuite) TestBuildFromOfficialNames(c *check.C) { } } -func (s *DockerSuite) TestBuildDockerfileOutsideContext(c *check.C) { - testRequires(c, UnixCli, DaemonIsLinux) // uses os.Symlink: not implemented in windows at the time of writing (go-1.4.2) - - name := "testbuilddockerfileoutsidecontext" - tmpdir, err := ioutil.TempDir("", name) - c.Assert(err, check.IsNil) - defer os.RemoveAll(tmpdir) - ctx := filepath.Join(tmpdir, "context") - if err := os.MkdirAll(ctx, 0755); err != nil { - c.Fatal(err) - } - if err := ioutil.WriteFile(filepath.Join(ctx, "Dockerfile"), []byte("FROM scratch\nENV X Y"), 0644); err != nil { - c.Fatal(err) - } - wd, err := os.Getwd() - if err != nil { - c.Fatal(err) - } - defer os.Chdir(wd) - if err := os.Chdir(ctx); err != nil { - c.Fatal(err) - } - if err := ioutil.WriteFile(filepath.Join(tmpdir, "outsideDockerfile"), []byte("FROM scratch\nENV x y"), 0644); err != nil { - c.Fatal(err) - } - if err := os.Symlink(filepath.Join("..", "outsideDockerfile"), filepath.Join(ctx, "dockerfile1")); err != nil { - c.Fatal(err) - } - if err := os.Symlink(filepath.Join(tmpdir, "outsideDockerfile"), filepath.Join(ctx, "dockerfile2")); err != nil { - c.Fatal(err) - } - - for _, dockerfilePath := range []string{ - filepath.Join("..", "outsideDockerfile"), - filepath.Join(ctx, "dockerfile1"), - filepath.Join(ctx, "dockerfile2"), - } { - result := dockerCmdWithResult("build", "-t", name, "--no-cache", "-f", dockerfilePath, ".") - result.Assert(c, icmd.Expected{ - Err: "must be within the build context", - ExitCode: 1, - }) - deleteImages(name) - } - - os.Chdir(tmpdir) - - // Path to Dockerfile should be resolved relative to working directory, not relative to context. - // There is a Dockerfile in the context, but since there is no Dockerfile in the current directory, the following should fail - out, _, err := dockerCmdWithError("build", "-t", name, "--no-cache", "-f", "Dockerfile", ctx) - if err == nil { - c.Fatalf("Expected error. Out: %s", out) - } -} - // FIXME(vdemeester) should be a unit test func (s *DockerSuite) TestBuildSpaces(c *check.C) { // Test to make sure that leading/trailing spaces on a command @@ -4186,6 +4083,7 @@ func (s *DockerTrustSuite) TestTrustedBuildUntrustedTag(c *check.C) { }) } +// FIXME(vdemeester) should migrate to docker/cli e2e tests func (s *DockerTrustSuite) TestBuildContextDirIsSymlink(c *check.C) { testRequires(c, DaemonIsLinux) tempDir, err := ioutil.TempDir("", "test-build-dir-is-symlink-") @@ -5132,6 +5030,7 @@ func (s *DockerSuite) TestBuildCacheRootSource(c *check.C) { } // #19375 +// FIXME(vdemeester) should migrate to docker/cli tests func (s *DockerSuite) TestBuildFailsGitNotCallable(c *check.C) { buildImage("gitnotcallable", cli.WithEnvironmentVariables("PATH="), build.WithContextPath("github.com/docker/v1.10-migrator.git")).Assert(c, icmd.Expected{ @@ -6449,6 +6348,7 @@ CMD echo foo c.Assert(strings.TrimSpace(out), checker.Equals, `["/bin/sh","-c","echo foo"]`) } +// FIXME(vdemeester) should migrate to docker/cli tests func (s *DockerSuite) TestBuildIidFile(c *check.C) { tmpDir, err := ioutil.TempDir("", "TestBuildIidFile") if err != nil { @@ -6473,6 +6373,7 @@ ENV BAR BAZ`), c.Assert(d.String(), checker.Equals, getIDByName(c, name)) } +// FIXME(vdemeester) should migrate to docker/cli tests func (s *DockerSuite) TestBuildIidFileCleanupOnFail(c *check.C) { tmpDir, err := ioutil.TempDir("", "TestBuildIidFileCleanupOnFail") if err != nil { @@ -6495,6 +6396,7 @@ func (s *DockerSuite) TestBuildIidFileCleanupOnFail(c *check.C) { c.Assert(os.IsNotExist(err), check.Equals, true) } +// FIXME(vdemeester) should migrate to docker/cli tests func (s *DockerSuite) TestBuildIidFileSquash(c *check.C) { testRequires(c, ExperimentalDaemon) tmpDir, err := ioutil.TempDir("", "TestBuildIidFileSquash") From 7d395933ee04cc9b981590d2f65cf0aa80fa9694 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 27 Feb 2018 16:38:02 -0500 Subject: [PATCH 2/4] Add more container cp tests Signed-off-by: Daniel Nephin (cherry picked from commit 07cb69e9bce40495b489905e6795dc048232b324) Signed-off-by: Sebastiaan van Stijn --- .../cli/cli/command/container/cp_test.go | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/components/cli/cli/command/container/cp_test.go b/components/cli/cli/command/container/cp_test.go index db7760c4b2..ce4430746e 100644 --- a/components/cli/cli/command/container/cp_test.go +++ b/components/cli/cli/command/container/cp_test.go @@ -3,6 +3,7 @@ package container import ( "io" "io/ioutil" + "os" "runtime" "strings" "testing" @@ -111,6 +112,33 @@ func TestRunCopyFromContainerToFilesystemMissingDestinationDirectory(t *testing. testutil.ErrorContains(t, err, destDir.Join("missing")) } +func TestRunCopyToContainerFromFileWithTrailingSlash(t *testing.T) { + srcFile := fs.NewFile(t, t.Name()) + defer srcFile.Remove() + + options := copyOptions{ + source: srcFile.Path() + string(os.PathSeparator), + destination: "container:/path", + } + cli := test.NewFakeCli(&fakeClient{}) + err := runCopy(cli, options) + testutil.ErrorContains(t, err, "not a directory") +} + +func TestRunCopyToContainerSourceDoesNotExist(t *testing.T) { + options := copyOptions{ + source: "/does/not/exist", + destination: "container:/path", + } + cli := test.NewFakeCli(&fakeClient{}) + err := runCopy(cli, options) + expected := "no such file or directory" + if runtime.GOOS == "windows" { + expected = "cannot find the file specified" + } + testutil.ErrorContains(t, err, expected) +} + func TestSplitCpArg(t *testing.T) { var testcases = []struct { doc string From 36343864e2a3d7fd669fc952c1b3b5b0a6dbb486 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 27 Feb 2018 16:46:14 -0500 Subject: [PATCH 3/4] Migrate some copy tests to integration Signed-off-by: Daniel Nephin (cherry picked from commit 00d409f03ed825f623b6ef8ec5a3a91cd26194c2) Signed-off-by: Sebastiaan van Stijn --- .../docker_cli_cp_from_container_test.go | 62 +-------- .../docker_cli_cp_to_container_test.go | 122 ------------------ .../docker_cli_cp_utils_test.go | 12 -- .../engine/integration/container/copy_test.go | 65 ++++++++++ 4 files changed, 66 insertions(+), 195 deletions(-) create mode 100644 components/engine/integration/container/copy_test.go diff --git a/components/engine/integration-cli/docker_cli_cp_from_container_test.go b/components/engine/integration-cli/docker_cli_cp_from_container_test.go index 0a282f5c09..499be54522 100644 --- a/components/engine/integration-cli/docker_cli_cp_from_container_test.go +++ b/components/engine/integration-cli/docker_cli_cp_from_container_test.go @@ -8,8 +8,6 @@ import ( "github.com/go-check/check" ) -// docker cp CONTAINER:PATH LOCALPATH - // Try all of the test cases from the archive package which implements the // internals of `docker cp` and ensure that the behavior matches when actually // copying to and from containers. @@ -20,67 +18,9 @@ import ( // 3. DST parent directory must exist. // 4. If DST exists as a file, it must not end with a trailing separator. -// First get these easy error cases out of the way. - -// Test for error when SRC does not exist. -func (s *DockerSuite) TestCpFromErrSrcNotExists(c *check.C) { - containerID := makeTestContainer(c, testContainerOptions{}) - - tmpDir := getTestDir(c, "test-cp-from-err-src-not-exists") - defer os.RemoveAll(tmpDir) - - err := runDockerCp(c, containerCpPath(containerID, "file1"), tmpDir, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCpNotExist(err), checker.True, check.Commentf("expected IsNotExist error, but got %T: %s", err, err)) -} - -// Test for error when SRC ends in a trailing -// path separator but it exists as a file. -func (s *DockerSuite) TestCpFromErrSrcNotDir(c *check.C) { - testRequires(c, DaemonIsLinux) - containerID := makeTestContainer(c, testContainerOptions{addContent: true}) - - tmpDir := getTestDir(c, "test-cp-from-err-src-not-dir") - defer os.RemoveAll(tmpDir) - - err := runDockerCp(c, containerCpPathTrailingSep(containerID, "file1"), tmpDir, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCpNotDir(err), checker.True, check.Commentf("expected IsNotDir error, but got %T: %s", err, err)) -} - -// Test for error when DST ends in a trailing -// path separator but exists as a file. -func (s *DockerSuite) TestCpFromErrDstNotDir(c *check.C) { - testRequires(c, DaemonIsLinux) - containerID := makeTestContainer(c, testContainerOptions{addContent: true}) - - tmpDir := getTestDir(c, "test-cp-from-err-dst-not-dir") - defer os.RemoveAll(tmpDir) - - makeTestContentInDir(c, tmpDir) - - // Try with a file source. - srcPath := containerCpPath(containerID, "/file1") - dstPath := cpPathTrailingSep(tmpDir, "file1") - - err := runDockerCp(c, srcPath, dstPath, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCpNotDir(err), checker.True, check.Commentf("expected IsNotDir error, but got %T: %s", err, err)) - - // Try with a directory source. - srcPath = containerCpPath(containerID, "/dir1") - - err = runDockerCp(c, srcPath, dstPath, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCpNotDir(err), checker.True, check.Commentf("expected IsNotDir error, but got %T: %s", err, err)) -} - // Check that copying from a container to a local symlink copies to the symlink // target and does not overwrite the local symlink itself. +// TODO: move to docker/cli and/or integration/container/copy_test.go func (s *DockerSuite) TestCpFromSymlinkDestination(c *check.C) { testRequires(c, DaemonIsLinux) containerID := makeTestContainer(c, testContainerOptions{addContent: true}) diff --git a/components/engine/integration-cli/docker_cli_cp_to_container_test.go b/components/engine/integration-cli/docker_cli_cp_to_container_test.go index 24c1fe2288..77567a3b95 100644 --- a/components/engine/integration-cli/docker_cli_cp_to_container_test.go +++ b/components/engine/integration-cli/docker_cli_cp_to_container_test.go @@ -2,15 +2,11 @@ package main import ( "os" - "runtime" - "strings" "github.com/docker/docker/integration-cli/checker" "github.com/go-check/check" ) -// docker cp LOCALPATH CONTAINER:PATH - // Try all of the test cases from the archive package which implements the // internals of `docker cp` and ensure that the behavior matches when actually // copying to and from containers. @@ -21,124 +17,6 @@ import ( // 3. DST parent directory must exist. // 4. If DST exists as a file, it must not end with a trailing separator. -// First get these easy error cases out of the way. - -// Test for error when SRC does not exist. -func (s *DockerSuite) TestCpToErrSrcNotExists(c *check.C) { - containerID := makeTestContainer(c, testContainerOptions{}) - - tmpDir := getTestDir(c, "test-cp-to-err-src-not-exists") - defer os.RemoveAll(tmpDir) - - srcPath := cpPath(tmpDir, "file1") - dstPath := containerCpPath(containerID, "file1") - _, srcStatErr := os.Stat(srcPath) - c.Assert(os.IsNotExist(srcStatErr), checker.True) - - err := runDockerCp(c, srcPath, dstPath, nil) - if runtime.GOOS == "windows" { - // Go 1.9+ on Windows returns a different error for `os.Stat()`, see - // https://github.com/golang/go/commit/6144c7270e5812d9de8fb97456ee4e5ae657fcbb#diff-f63e1a4b4377b2fe0b05011db3df9599 - // - // Go 1.8: CreateFile C:\not-exist: The system cannot find the file specified. - // Go 1.9: GetFileAttributesEx C:\not-exist: The system cannot find the file specified. - // - // Due to the CLI using a different version than the daemon, comparing the - // error message won't work, so just hard-code the common part here. - // - // TODO this should probably be a test in the CLI repository instead - c.Assert(strings.ToLower(err.Error()), checker.Contains, "cannot find the file specified") - c.Assert(strings.ToLower(err.Error()), checker.Contains, strings.ToLower(tmpDir)) - } else { - c.Assert(strings.ToLower(err.Error()), checker.Contains, strings.ToLower(srcStatErr.Error())) - } -} - -// Test for error when SRC ends in a trailing -// path separator but it exists as a file. -func (s *DockerSuite) TestCpToErrSrcNotDir(c *check.C) { - containerID := makeTestContainer(c, testContainerOptions{}) - - tmpDir := getTestDir(c, "test-cp-to-err-src-not-dir") - defer os.RemoveAll(tmpDir) - - makeTestContentInDir(c, tmpDir) - - srcPath := cpPathTrailingSep(tmpDir, "file1") - dstPath := containerCpPath(containerID, "testDir") - - err := runDockerCp(c, srcPath, dstPath, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCpNotDir(err), checker.True, check.Commentf("expected IsNotDir error, but got %T: %s", err, err)) -} - -// Test for error when SRC is a valid file or directory, -// but the DST parent directory does not exist. -func (s *DockerSuite) TestCpToErrDstParentNotExists(c *check.C) { - testRequires(c, DaemonIsLinux) - containerID := makeTestContainer(c, testContainerOptions{addContent: true}) - - tmpDir := getTestDir(c, "test-cp-to-err-dst-parent-not-exists") - defer os.RemoveAll(tmpDir) - - makeTestContentInDir(c, tmpDir) - - // Try with a file source. - srcPath := cpPath(tmpDir, "file1") - dstPath := containerCpPath(containerID, "/notExists", "file1") - - err := runDockerCp(c, srcPath, dstPath, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCpNotExist(err), checker.True, check.Commentf("expected IsNotExist error, but got %T: %s", err, err)) - - // Try with a directory source. - srcPath = cpPath(tmpDir, "dir1") - - err = runDockerCp(c, srcPath, dstPath, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCpNotExist(err), checker.True, check.Commentf("expected IsNotExist error, but got %T: %s", err, err)) -} - -// Test for error when DST ends in a trailing path separator but exists as a -// file. Also test that we cannot overwrite an existing directory with a -// non-directory and cannot overwrite an existing -func (s *DockerSuite) TestCpToErrDstNotDir(c *check.C) { - testRequires(c, DaemonIsLinux) - containerID := makeTestContainer(c, testContainerOptions{addContent: true}) - - tmpDir := getTestDir(c, "test-cp-to-err-dst-not-dir") - defer os.RemoveAll(tmpDir) - - makeTestContentInDir(c, tmpDir) - - // Try with a file source. - srcPath := cpPath(tmpDir, "dir1/file1-1") - dstPath := containerCpPathTrailingSep(containerID, "file1") - - // The client should encounter an error trying to stat the destination - // and then be unable to copy since the destination is asserted to be a - // directory but does not exist. - err := runDockerCp(c, srcPath, dstPath, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCpDirNotExist(err), checker.True, check.Commentf("expected DirNotExist error, but got %T: %s", err, err)) - - // Try with a directory source. - srcPath = cpPath(tmpDir, "dir1") - - // The client should encounter an error trying to stat the destination and - // then decide to extract to the parent directory instead with a rebased - // name in the source archive, but this directory would overwrite the - // existing file with the same name. - err = runDockerCp(c, srcPath, dstPath, nil) - c.Assert(err, checker.NotNil) - - c.Assert(isCannotOverwriteNonDirWithDir(err), checker.True, check.Commentf("expected CannotOverwriteNonDirWithDir error, but got %T: %s", err, err)) -} - // Check that copying from a local path to a symlink in a container copies to // the symlink target and does not overwrite the container symlink itself. func (s *DockerSuite) TestCpToSymlinkDestination(c *check.C) { diff --git a/components/engine/integration-cli/docker_cli_cp_utils_test.go b/components/engine/integration-cli/docker_cli_cp_utils_test.go index 402a87ea90..79a016f0c6 100644 --- a/components/engine/integration-cli/docker_cli_cp_utils_test.go +++ b/components/engine/integration-cli/docker_cli_cp_utils_test.go @@ -228,18 +228,10 @@ func getTestDir(c *check.C, label string) (tmpDir string) { return } -func isCpNotExist(err error) bool { - return strings.Contains(strings.ToLower(err.Error()), "could not find the file") -} - func isCpDirNotExist(err error) bool { return strings.Contains(err.Error(), archive.ErrDirNotExists.Error()) } -func isCpNotDir(err error) bool { - return strings.Contains(err.Error(), archive.ErrNotDirectory.Error()) || strings.Contains(err.Error(), "filename, directory name, or volume label syntax is incorrect") -} - func isCpCannotCopyDir(err error) bool { return strings.Contains(err.Error(), archive.ErrCannotCopyDir.Error()) } @@ -248,10 +240,6 @@ func isCpCannotCopyReadOnly(err error) bool { return strings.Contains(err.Error(), "marked read-only") } -func isCannotOverwriteNonDirWithDir(err error) bool { - return strings.Contains(err.Error(), "cannot overwrite non-directory") -} - func fileContentEquals(c *check.C, filename, contents string) (err error) { c.Logf("checking that file %q contains %q\n", filename, contents) diff --git a/components/engine/integration/container/copy_test.go b/components/engine/integration/container/copy_test.go new file mode 100644 index 0000000000..43dc31f2f2 --- /dev/null +++ b/components/engine/integration/container/copy_test.go @@ -0,0 +1,65 @@ +package container // import "github.com/docker/docker/integration/container" + +import ( + "context" + "fmt" + "testing" + + "github.com/docker/docker/api/types" + "github.com/docker/docker/client" + "github.com/docker/docker/integration/internal/container" + "github.com/docker/docker/internal/testutil" + "github.com/gotestyourself/gotestyourself/skip" + "github.com/stretchr/testify/require" +) + +func TestCopyFromContainerPathDoesNotExist(t *testing.T) { + defer setupTest(t)() + + ctx := context.Background() + apiclient := testEnv.APIClient() + cid := container.Create(t, ctx, apiclient) + + _, _, err := apiclient.CopyFromContainer(ctx, cid, "/dne") + require.True(t, client.IsErrNotFound(err)) + expected := fmt.Sprintf("No such container:path: %s:%s", cid, "/dne") + testutil.ErrorContains(t, err, expected) +} + +func TestCopyFromContainerPathIsNotDir(t *testing.T) { + defer setupTest(t)() + skip.If(t, testEnv.OSType == "windows") + + ctx := context.Background() + apiclient := testEnv.APIClient() + cid := container.Create(t, ctx, apiclient) + + _, _, err := apiclient.CopyFromContainer(ctx, cid, "/etc/passwd/") + require.Contains(t, err.Error(), "not a directory") +} + +func TestCopyToContainerPathDoesNotExist(t *testing.T) { + defer setupTest(t)() + skip.If(t, testEnv.OSType == "windows") + + ctx := context.Background() + apiclient := testEnv.APIClient() + cid := container.Create(t, ctx, apiclient) + + err := apiclient.CopyToContainer(ctx, cid, "/dne", nil, types.CopyToContainerOptions{}) + require.True(t, client.IsErrNotFound(err)) + expected := fmt.Sprintf("No such container:path: %s:%s", cid, "/dne") + testutil.ErrorContains(t, err, expected) +} + +func TestCopyToContainerPathIsNotDir(t *testing.T) { + defer setupTest(t)() + skip.If(t, testEnv.OSType == "windows") + + ctx := context.Background() + apiclient := testEnv.APIClient() + cid := container.Create(t, ctx, apiclient) + + err := apiclient.CopyToContainer(ctx, cid, "/etc/passwd/", nil, types.CopyToContainerOptions{}) + require.Contains(t, err.Error(), "not a directory") +} From 50c9a31b4c27c6e3d44f24c2f88dec59809f3b2f Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Tue, 27 Feb 2018 12:14:07 +0100 Subject: [PATCH 4/4] Fix `--label` behavior on run Commit 2b17f4c8a8caad552025edb05a73db683fb8a5c6 fixed the way empty labels are taken into account (i.e. not interpolated from environment variable), but it created a regression. `ValidateLabel` functions doesn't allow empty label value, but it has always been possible to pass an empty label via the cli (`docker run --label foo`). This fixes that by not validating the label flag. Signed-off-by: Vincent Demeester (cherry picked from commit 31dc5c0a9a8bdc11c7ad335aebb753ed527caa5a) Signed-off-by: Vincent Demeester --- .../cli/cli/command/container/client_test.go | 8 ++++++ components/cli/cli/command/container/opts.go | 2 +- .../cli/cli/command/container/run_test.go | 25 +++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 components/cli/cli/command/container/run_test.go diff --git a/components/cli/cli/command/container/client_test.go b/components/cli/cli/command/container/client_test.go index e0ffd2a66e..ed04920b16 100644 --- a/components/cli/cli/command/container/client_test.go +++ b/components/cli/cli/command/container/client_test.go @@ -16,6 +16,7 @@ type fakeClient struct { execInspectFunc func(execID string) (types.ContainerExecInspect, error) execCreateFunc func(container string, config types.ExecConfig) (types.IDResponse, error) createContainerFunc func(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, containerName string) (container.ContainerCreateCreatedBody, error) + containerStartFunc func(container string, options types.ContainerStartOptions) error imageCreateFunc func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) infoFunc func() (types.Info, error) containerStatPathFunc func(container, path string) (types.ContainerPathStat, error) @@ -116,3 +117,10 @@ func (f *fakeClient) ContainerWait(_ context.Context, container string, _ contai } return nil, nil } + +func (f *fakeClient) ContainerStart(_ context.Context, container string, options types.ContainerStartOptions) error { + if f.containerStartFunc != nil { + return f.containerStartFunc(container, options) + } + return nil +} diff --git a/components/cli/cli/command/container/opts.go b/components/cli/cli/command/container/opts.go index 0adecca759..b8ff5e4a4a 100644 --- a/components/cli/cli/command/container/opts.go +++ b/components/cli/cli/command/container/opts.go @@ -145,7 +145,7 @@ func addFlags(flags *pflag.FlagSet) *containerOptions { expose: opts.NewListOpts(nil), extraHosts: opts.NewListOpts(opts.ValidateExtraHost), groupAdd: opts.NewListOpts(nil), - labels: opts.NewListOpts(opts.ValidateLabel), + labels: opts.NewListOpts(nil), labelsFile: opts.NewListOpts(nil), linkLocalIPs: opts.NewListOpts(nil), links: opts.NewListOpts(opts.ValidateLink), diff --git a/components/cli/cli/command/container/run_test.go b/components/cli/cli/command/container/run_test.go new file mode 100644 index 0000000000..ca08fb3362 --- /dev/null +++ b/components/cli/cli/command/container/run_test.go @@ -0,0 +1,25 @@ +package container + +import ( + "testing" + + "github.com/docker/cli/internal/test" + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/network" + "github.com/stretchr/testify/assert" +) + +func TestRunLabel(t *testing.T) { + cli := test.NewFakeCli(&fakeClient{ + createContainerFunc: func(_ *container.Config, _ *container.HostConfig, _ *network.NetworkingConfig, _ string) (container.ContainerCreateCreatedBody, error) { + return container.ContainerCreateCreatedBody{ + ID: "id", + }, nil + }, + Version: "1.36", + }) + cmd := NewRunCommand(cli) + cmd.Flags().Set("detach", "true") + cmd.SetArgs([]string{"--label", "foo", "busybox"}) + assert.NoError(t, cmd.Execute()) +}