From cc0f59742f192ccbc178c59bdc7eb98d0f6dd943 Mon Sep 17 00:00:00 2001 From: Solomon Hykes Date: Thu, 20 Jun 2013 20:16:39 -0700 Subject: [PATCH 1/2] * Builder: simplified unit tests. The tests are now embedded in the build itself. Yeah baby. --- buildfile_test.go | 48 ++++++----------------------------------------- 1 file changed, 6 insertions(+), 42 deletions(-) diff --git a/buildfile_test.go b/buildfile_test.go index 33e6a3146b..773470b809 100644 --- a/buildfile_test.go +++ b/buildfile_test.go @@ -13,6 +13,8 @@ const Dockerfile = ` from ` + unitTestImageName + ` run sh -c 'echo root:testpass > /tmp/passwd' run mkdir -p /var/run/sshd +run [ "$(cat /tmp/passwd)" = "root:testpass" ] +run [ "$(ls -d /var/run/sshd)" = "/var/run/sshd" ] ` const DockerfileNoNewLine = ` @@ -21,7 +23,9 @@ const DockerfileNoNewLine = ` from ` + unitTestImageName + ` run sh -c 'echo root:testpass > /tmp/passwd' -run mkdir -p /var/run/sshd` +run mkdir -p /var/run/sshd +run [ "$(cat /tmp/passwd)" = "root:testpass" ] +run [ "$(ls -d /var/run/sshd)" = "/var/run/sshd" ]` // FIXME: test building with a context @@ -42,48 +46,8 @@ func TestBuild(t *testing.T) { buildfile := NewBuildFile(srv, &utils.NopWriter{}) - imgID, err := buildfile.Build(strings.NewReader(Dockerfile), nil) - if err != nil { + if _, err := buildfile.Build(strings.NewReader(Dockerfile), nil); err != nil { t.Fatal(err) } - - builder := NewBuilder(runtime) - container, err := builder.Create( - &Config{ - Image: imgID, - Cmd: []string{"cat", "/tmp/passwd"}, - }, - ) - if err != nil { - t.Fatal(err) - } - defer runtime.Destroy(container) - - output, err := container.Output() - if err != nil { - t.Fatal(err) - } - if string(output) != "root:testpass\n" { - t.Fatalf("Unexpected output. Read '%s', expected '%s'", output, "root:testpass\n") - } - - container2, err := builder.Create( - &Config{ - Image: imgID, - Cmd: []string{"ls", "-d", "/var/run/sshd"}, - }, - ) - if err != nil { - t.Fatal(err) - } - defer runtime.Destroy(container2) - - output, err = container2.Output() - if err != nil { - t.Fatal(err) - } - if string(output) != "/var/run/sshd\n" { - t.Fatal("/var/run/sshd has not been created") - } } } From 36d610a38806feb4cb5d09c4705a6f1fff42ef04 Mon Sep 17 00:00:00 2001 From: Solomon Hykes Date: Thu, 20 Jun 2013 20:20:16 -0700 Subject: [PATCH 2/2] - Builder: fixed a regression in ADD. Improved regression tests for build behavior. --- archive.go | 98 +++++++++++++++++++++----------------- hack/test-build/Dockerfile | 17 +++++++ hack/test-build/README.md | 9 ++++ hack/test-build/d/ga | 1 + hack/test-build/f | 1 + 5 files changed, 83 insertions(+), 43 deletions(-) create mode 100644 hack/test-build/Dockerfile create mode 100644 hack/test-build/README.md create mode 100644 hack/test-build/d/ga create mode 100644 hack/test-build/f diff --git a/archive.go b/archive.go index 16401e29fb..5756490bff 100644 --- a/archive.go +++ b/archive.go @@ -1,7 +1,9 @@ package docker import ( + "archive/tar" "bufio" + "bytes" "errors" "fmt" "github.com/dotcloud/docker/utils" @@ -10,6 +12,7 @@ import ( "os" "os/exec" "path" + "path/filepath" ) type Archive io.Reader @@ -160,51 +163,60 @@ func CopyWithTar(src, dst string) error { if err != nil { return err } - var dstExists bool - dstSt, err := os.Stat(dst) - if err != nil { - if !os.IsNotExist(err) { - return err - } - } else { - dstExists = true - } - // Things that can go wrong if the source is a directory - if srcSt.IsDir() { - // The destination exists and is a regular file - if dstExists && !dstSt.IsDir() { - return fmt.Errorf("Can't copy a directory over a regular file") - } - // Things that can go wrong if the source is a regular file - } else { - utils.Debugf("The destination exists, it's a directory, and doesn't end in /") - // The destination exists, it's a directory, and doesn't end in / - if dstExists && dstSt.IsDir() && dst[len(dst)-1] != '/' { - return fmt.Errorf("Can't copy a regular file over a directory %s |%s|", dst, dst[len(dst)-1]) - } - } - // Create the destination - var dstDir string - if srcSt.IsDir() || dst[len(dst)-1] == '/' { - // The destination ends in /, or the source is a directory - // --> dst is the holding directory and needs to be created for -C - dstDir = dst - } else { - // The destination doesn't end in / - // --> dst is the file - dstDir = path.Dir(dst) - } - if !dstExists { - // Create the holding directory if necessary - utils.Debugf("Creating the holding directory %s", dstDir) - if err := os.MkdirAll(dstDir, 0700); err != nil && !os.IsExist(err) { - return err - } - } if !srcSt.IsDir() { - return TarUntar(path.Dir(src), []string{path.Base(src)}, dstDir) + return CopyFileWithTar(src, dst) } - return TarUntar(src, nil, dstDir) + // Create dst, copy src's content into it + utils.Debugf("Creating dest directory: %s", dst) + if err := os.MkdirAll(dst, 0700); err != nil && !os.IsExist(err) { + return err + } + utils.Debugf("Calling TarUntar(%s, %s)", src, dst) + return TarUntar(src, nil, dst) +} + +// CopyFileWithTar emulates the behavior of the 'cp' command-line +// for a single file. It copies a regular file from path `src` to +// path `dst`, and preserves all its metadata. +// +// If `dst` ends with a trailing slash '/', the final destination path +// will be `dst/base(src)`. +func CopyFileWithTar(src, dst string) error { + utils.Debugf("CopyFileWithTar(%s, %s)", src, dst) + srcSt, err := os.Stat(src) + if err != nil { + return err + } + if srcSt.IsDir() { + return fmt.Errorf("Can't copy a directory") + } + // Clean up the trailing / + if dst[len(dst)-1] == '/' { + dst = path.Join(dst, filepath.Base(src)) + } + // Create the holding directory if necessary + if err := os.MkdirAll(filepath.Dir(dst), 0700); err != nil && !os.IsExist(err) { + return err + } + buf := new(bytes.Buffer) + tw := tar.NewWriter(buf) + hdr, err := tar.FileInfoHeader(srcSt, "") + if err != nil { + return err + } + hdr.Name = filepath.Base(dst) + if err := tw.WriteHeader(hdr); err != nil { + return err + } + srcF, err := os.Open(src) + if err != nil { + return err + } + if _, err := io.Copy(tw, srcF); err != nil { + return err + } + tw.Close() + return Untar(buf, filepath.Dir(dst)) } // CmdStream executes a command, and returns its stdout as a stream. diff --git a/hack/test-build/Dockerfile b/hack/test-build/Dockerfile new file mode 100644 index 0000000000..a232253c9c --- /dev/null +++ b/hack/test-build/Dockerfile @@ -0,0 +1,17 @@ +from busybox +add f / +run [ "$(cat /f)" = "hello" ] +add f /abc +run [ "$(cat /abc)" = "hello" ] +add f /x/y/z +run [ "$(cat /x/y/z)" = "hello" ] +add f /x/y/d/ +run [ "$(cat /x/y/d/f)" = "hello" ] +add d / +run [ "$(cat /ga)" = "bu" ] +add d /somewhere +run [ "$(cat /somewhere/ga)" = "bu" ] +add d /anotherplace/ +run [ "$(cat /anotherplace/ga)" = "bu" ] +add d /somewheeeere/over/the/rainbooow +run [ "$(cat /somewheeeere/over/the/rainbooow/ga)" = "bu" ] diff --git a/hack/test-build/README.md b/hack/test-build/README.md new file mode 100644 index 0000000000..77636dcda6 --- /dev/null +++ b/hack/test-build/README.md @@ -0,0 +1,9 @@ +This directory is meant to test the behavior of 'docker build' in general, and its ADD command in particular. +Ideally it would be automatically called as part of the unit test suite - but that requires slightly more glue, +so for now we're just calling it manually, because it's better than nothing. + +To test, simply build this directory with the following command: + + docker build . + +The tests are embedded in the Dockerfile: if the build succeeds, the tests have passed. diff --git a/hack/test-build/d/ga b/hack/test-build/d/ga new file mode 100644 index 0000000000..7e6edb8302 --- /dev/null +++ b/hack/test-build/d/ga @@ -0,0 +1 @@ +bu diff --git a/hack/test-build/f b/hack/test-build/f new file mode 100644 index 0000000000..ce01362503 --- /dev/null +++ b/hack/test-build/f @@ -0,0 +1 @@ +hello