Adding unit tests for cp

Signed-off-by: Daniel Nephin <dnephin@docker.com>

cli/command/container/client_test.go

@@ -12,12 +12,14 @@ import (
 
 type fakeClient struct {
 	client.Client
-	inspectFunc         func(string) (types.ContainerJSON, error)
+	inspectFunc           func(string) (types.ContainerJSON, error)
-	execInspectFunc     func(execID string) (types.ContainerExecInspect, error)
+	execInspectFunc       func(execID string) (types.ContainerExecInspect, error)
-	execCreateFunc      func(container string, config types.ExecConfig) (types.IDResponse, 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)
+	createContainerFunc   func(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, containerName string) (container.ContainerCreateCreatedBody, error)
-	imageCreateFunc     func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error)
+	imageCreateFunc       func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error)
-	infoFunc            func() (types.Info, error)
+	infoFunc              func() (types.Info, error)
+	containerStatPathFunc func(container, path string) (types.ContainerPathStat, error)
+	containerCopyFromFunc func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error)
 }
 
 func (f *fakeClient) ContainerInspect(_ context.Context, containerID string) (types.ContainerJSON, error) {
@@ -71,3 +73,17 @@ func (f *fakeClient) Info(_ context.Context) (types.Info, error) {
 	}
 	return types.Info{}, nil
 }
+
+func (f *fakeClient) ContainerStatPath(_ context.Context, container, path string) (types.ContainerPathStat, error) {
+	if f.containerStatPathFunc != nil {
+		return f.containerStatPathFunc(container, path)
+	}
+	return types.ContainerPathStat{}, nil
+}
+
+func (f *fakeClient) CopyFromContainer(_ context.Context, container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) {
+	if f.containerCopyFromFunc != nil {
+		return f.containerCopyFromFunc(container, srcPath)
+	}
+	return nil, types.ContainerPathStat{}, nil
+}

cli/command/container/cp.go

@@ -26,13 +26,17 @@ type copyOptions struct {
 type copyDirection int
 
 const (
-	fromContainer copyDirection = (1 << iota)
+	fromContainer copyDirection = 1 << iota
 	toContainer
 	acrossContainers = fromContainer | toContainer
 )
 
 type cpConfig struct {
 	followLink bool
+	copyUIDGID bool
+	sourcePath string
+	destPath   string
+	container  string
 }
 
 // NewCopyCommand creates a new `docker cp` command
@@ -65,58 +69,57 @@ func NewCopyCommand(dockerCli command.Cli) *cobra.Command {
 	}
 
 	flags := cmd.Flags()
-
 	flags.BoolVarP(&opts.followLink, "follow-link", "L", false, "Always follow symbol link in SRC_PATH")
 	flags.BoolVarP(&opts.copyUIDGID, "archive", "a", false, "Archive mode (copy all uid/gid information)")
-
 	return cmd
 }
 
 func runCopy(dockerCli command.Cli, opts copyOptions) error {
 	srcContainer, srcPath := splitCpArg(opts.source)
-	dstContainer, dstPath := splitCpArg(opts.destination)
+	destContainer, destPath := splitCpArg(opts.destination)
+
+	copyConfig := cpConfig{
+		followLink: opts.followLink,
+		copyUIDGID: opts.copyUIDGID,
+		sourcePath: srcPath,
+		destPath:   destPath,
+	}
 
 	var direction copyDirection
 	if srcContainer != "" {
 		direction |= fromContainer
+		copyConfig.container = srcContainer
 	}
-	if dstContainer != "" {
+	if destContainer != "" {
 		direction |= toContainer
-	}
+		copyConfig.container = destContainer
-
-	cpParam := &cpConfig{
-		followLink: opts.followLink,
 	}
 
 	ctx := context.Background()
 
 	switch direction {
 	case fromContainer:
-		return copyFromContainer(ctx, dockerCli, srcContainer, srcPath, dstPath, cpParam)
+		return copyFromContainer(ctx, dockerCli, copyConfig)
 	case toContainer:
-		return copyToContainer(ctx, dockerCli, srcPath, dstContainer, dstPath, cpParam, opts.copyUIDGID)
+		return copyToContainer(ctx, dockerCli, copyConfig)
 	case acrossContainers:
-		// Copying between containers isn't supported.
 		return errors.New("copying between containers is not supported")
 	default:
-		// User didn't specify any container.
 		return errors.New("must specify at least one container source")
 	}
 }
 
-func statContainerPath(ctx context.Context, dockerCli command.Cli, containerName, path string) (types.ContainerPathStat, error) {
-	return dockerCli.Client().ContainerStatPath(ctx, containerName, path)
-}
-
 func resolveLocalPath(localPath string) (absPath string, err error) {
 	if absPath, err = filepath.Abs(localPath); err != nil {
 		return
 	}
-
 	return archive.PreserveTrailingDotOrSeparator(absPath, localPath, filepath.Separator), nil
 }
 
-func copyFromContainer(ctx context.Context, dockerCli command.Cli, srcContainer, srcPath, dstPath string, cpParam *cpConfig) (err error) {
+func copyFromContainer(ctx context.Context, dockerCli command.Cli, copyConfig cpConfig) (err error) {
+	dstPath := copyConfig.destPath
+	srcPath := copyConfig.sourcePath
+
 	if dstPath != "-" {
 		// Get an absolute destination path.
 		dstPath, err = resolveLocalPath(dstPath)
@@ -125,10 +128,11 @@ func copyFromContainer(ctx context.Context, dockerCli command.Cli, srcContainer,
 		}
 	}
 
+	client := dockerCli.Client()
 	// if client requests to follow symbol link, then must decide target file to be copied
 	var rebaseName string
-	if cpParam.followLink {
+	if copyConfig.followLink {
-		srcStat, err := statContainerPath(ctx, dockerCli, srcContainer, srcPath)
+		srcStat, err := client.ContainerStatPath(ctx, copyConfig.container, srcPath)
 
 		// If the destination is a symbolic link, we should follow it.
 		if err == nil && srcStat.Mode&os.ModeSymlink != 0 {
@@ -145,20 +149,17 @@ func copyFromContainer(ctx context.Context, dockerCli command.Cli, srcContainer,
 
 	}
 
-	content, stat, err := dockerCli.Client().CopyFromContainer(ctx, srcContainer, srcPath)
+	content, stat, err := client.CopyFromContainer(ctx, copyConfig.container, srcPath)
 	if err != nil {
 		return err
 	}
 	defer content.Close()
 
 	if dstPath == "-" {
-		// Send the response to STDOUT.
+		_, err = io.Copy(dockerCli.Out(), content)
-		_, err = io.Copy(os.Stdout, content)
-
 		return err
 	}
 
-	// Prepare source copy info.
 	srcInfo := archive.CopyInfo{
 		Path:       srcPath,
 		Exists:     true,
@@ -171,13 +172,17 @@ func copyFromContainer(ctx context.Context, dockerCli command.Cli, srcContainer,
 		_, srcBase := archive.SplitPathDirEntry(srcInfo.Path)
 		preArchive = archive.RebaseArchiveEntries(content, srcBase, srcInfo.RebaseName)
 	}
-	// See comments in the implementation of `archive.CopyTo` for exactly what
-	// goes into deciding how and whether the source archive needs to be
-	// altered for the correct copy behavior.
 	return archive.CopyTo(preArchive, srcInfo, dstPath)
 }
 
-func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstContainer, dstPath string, cpParam *cpConfig, copyUIDGID bool) (err error) {
+// In order to get the copy behavior right, we need to know information
+// about both the source and destination. The API is a simple tar
+// archive/extract API but we can use the stat info header about the
+// destination to be more informed about exactly what the destination is.
+func copyToContainer(ctx context.Context, dockerCli command.Cli, copyConfig cpConfig) (err error) {
+	srcPath := copyConfig.sourcePath
+	dstPath := copyConfig.destPath
+
 	if srcPath != "-" {
 		// Get an absolute source path.
 		srcPath, err = resolveLocalPath(srcPath)
@@ -186,14 +191,10 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstCon
 		}
 	}
 
-	// In order to get the copy behavior right, we need to know information
+	client := dockerCli.Client()
-	// about both the source and destination. The API is a simple tar
-	// archive/extract API but we can use the stat info header about the
-	// destination to be more informed about exactly what the destination is.
-
 	// Prepare destination copy info by stat-ing the container path.
 	dstInfo := archive.CopyInfo{Path: dstPath}
-	dstStat, err := statContainerPath(ctx, dockerCli, dstContainer, dstPath)
+	dstStat, err := client.ContainerStatPath(ctx, copyConfig.container, dstPath)
 
 	// If the destination is a symbolic link, we should evaluate it.
 	if err == nil && dstStat.Mode&os.ModeSymlink != 0 {
@@ -205,7 +206,7 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstCon
 		}
 
 		dstInfo.Path = linkTarget
-		dstStat, err = statContainerPath(ctx, dockerCli, dstContainer, linkTarget)
+		dstStat, err = client.ContainerStatPath(ctx, copyConfig.container, linkTarget)
 	}
 
 	// Ignore any error and assume that the parent directory of the destination
@@ -224,15 +225,14 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstCon
 	)
 
 	if srcPath == "-" {
-		// Use STDIN.
 		content = os.Stdin
 		resolvedDstPath = dstInfo.Path
 		if !dstInfo.IsDir {
-			return errors.Errorf("destination \"%s:%s\" must be a directory", dstContainer, dstPath)
+			return errors.Errorf("destination \"%s:%s\" must be a directory", copyConfig.container, dstPath)
 		}
 	} else {
 		// Prepare source copy info.
-		srcInfo, err := archive.CopyInfoSourcePath(srcPath, cpParam.followLink)
+		srcInfo, err := archive.CopyInfoSourcePath(srcPath, copyConfig.followLink)
 		if err != nil {
 			return err
 		}
@@ -267,10 +267,9 @@ func copyToContainer(ctx context.Context, dockerCli command.Cli, srcPath, dstCon
 
 	options := types.CopyToContainerOptions{
 		AllowOverwriteDirWithFile: false,
-		CopyUIDGID:                copyUIDGID,
+		CopyUIDGID:                copyConfig.copyUIDGID,
 	}
-
+	return client.CopyToContainer(ctx, copyConfig.container, resolvedDstPath, content, options)
-	return dockerCli.Client().CopyToContainer(ctx, dstContainer, resolvedDstPath, content, options)
 }
 
 // We use `:` as a delimiter between CONTAINER and PATH, but `:` could also be

cli/command/container/cp_test.go

@@ -0,0 +1,160 @@
+package container
+
+import (
+	"io"
+	"io/ioutil"
+	"runtime"
+	"strings"
+	"testing"
+
+	"github.com/docker/cli/internal/test"
+	"github.com/docker/cli/internal/test/testutil"
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/pkg/archive"
+	"github.com/gotestyourself/gotestyourself/fs"
+	"github.com/gotestyourself/gotestyourself/skip"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestRunCopyWithInvalidArguments(t *testing.T) {
+	var testcases = []struct {
+		doc         string
+		options     copyOptions
+		expectedErr string
+	}{
+		{
+			doc: "copy between container",
+			options: copyOptions{
+				source:      "first:/path",
+				destination: "second:/path",
+			},
+			expectedErr: "copying between containers is not supported",
+		},
+		{
+			doc: "copy without a container",
+			options: copyOptions{
+				source:      "./source",
+				destination: "./dest",
+			},
+			expectedErr: "must specify at least one container source",
+		},
+	}
+	for _, testcase := range testcases {
+		t.Run(testcase.doc, func(t *testing.T) {
+			err := runCopy(test.NewFakeCli(nil), testcase.options)
+			assert.EqualError(t, err, testcase.expectedErr)
+		})
+	}
+}
+
+func TestRunCopyFromContainerToStdout(t *testing.T) {
+	tarContent := "the tar content"
+
+	fakeClient := &fakeClient{
+		containerCopyFromFunc: func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) {
+			assert.Equal(t, "container", container)
+			return ioutil.NopCloser(strings.NewReader(tarContent)), types.ContainerPathStat{}, nil
+		},
+	}
+	options := copyOptions{source: "container:/path", destination: "-"}
+	cli := test.NewFakeCli(fakeClient)
+	err := runCopy(cli, options)
+	require.NoError(t, err)
+	assert.Equal(t, tarContent, cli.OutBuffer().String())
+	assert.Equal(t, "", cli.ErrBuffer().String())
+}
+
+func TestRunCopyFromContainerToFilesystem(t *testing.T) {
+	destDir := fs.NewDir(t, "cp-test",
+		fs.WithFile("file1", "content\n"))
+	defer destDir.Remove()
+
+	fakeClient := &fakeClient{
+		containerCopyFromFunc: func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) {
+			assert.Equal(t, "container", container)
+			readCloser, err := archive.TarWithOptions(destDir.Path(), &archive.TarOptions{})
+			return readCloser, types.ContainerPathStat{}, err
+		},
+	}
+	options := copyOptions{source: "container:/path", destination: destDir.Path()}
+	cli := test.NewFakeCli(fakeClient)
+	err := runCopy(cli, options)
+	require.NoError(t, err)
+	assert.Equal(t, "", cli.OutBuffer().String())
+	assert.Equal(t, "", cli.ErrBuffer().String())
+
+	content, err := ioutil.ReadFile(destDir.Join("file1"))
+	require.NoError(t, err)
+	assert.Equal(t, "content\n", string(content))
+}
+
+func TestRunCopyFromContainerToFilesystemMissingDestinationDirectory(t *testing.T) {
+	destDir := fs.NewDir(t, "cp-test",
+		fs.WithFile("file1", "content\n"))
+	defer destDir.Remove()
+
+	fakeClient := &fakeClient{
+		containerCopyFromFunc: func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) {
+			assert.Equal(t, "container", container)
+			readCloser, err := archive.TarWithOptions(destDir.Path(), &archive.TarOptions{})
+			return readCloser, types.ContainerPathStat{}, err
+		},
+	}
+
+	options := copyOptions{
+		source:      "container:/path",
+		destination: destDir.Join("missing", "foo"),
+	}
+	cli := test.NewFakeCli(fakeClient)
+	err := runCopy(cli, options)
+	testutil.ErrorContains(t, err, destDir.Join("missing"))
+}
+
+func TestSplitCpArg(t *testing.T) {
+	var testcases = []struct {
+		doc               string
+		path              string
+		os                string
+		expectedContainer string
+		expectedPath      string
+	}{
+		{
+			doc:          "absolute path with colon",
+			os:           "linux",
+			path:         "/abs/path:withcolon",
+			expectedPath: "/abs/path:withcolon",
+		},
+		{
+			doc:          "relative path with colon",
+			path:         "./relative:path",
+			expectedPath: "./relative:path",
+		},
+		{
+			doc:          "absolute path with drive",
+			os:           "windows",
+			path:         `d:\abs\path`,
+			expectedPath: `d:\abs\path`,
+		},
+		{
+			doc:          "no separator",
+			path:         "relative/path",
+			expectedPath: "relative/path",
+		},
+		{
+			doc:               "with separator",
+			path:              "container:/opt/foo",
+			expectedPath:      "/opt/foo",
+			expectedContainer: "container",
+		},
+	}
+	for _, testcase := range testcases {
+		t.Run(testcase.doc, func(t *testing.T) {
+			skip.IfCondition(t, testcase.os != "" && testcase.os != runtime.GOOS)
+
+			container, path := splitCpArg(testcase.path)
+			assert.Equal(t, testcase.expectedContainer, container)
+			assert.Equal(t, testcase.expectedPath, path)
+		})
+	}
+}