Cleanup handling of the - for stdin

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

cli/command/image/build.go

@@ -67,6 +67,18 @@ type buildOptions struct {
 	target         string
 }
 
+// dockerfileFromStdin returns true when the user specified that the Dockerfile
+// should be read from stdin instead of a file
+func (o buildOptions) dockerfileFromStdin() bool {
+	return o.dockerfileName == "-"
+}
+
+// contextFromStdin returns true when the user specified that the build context
+// should be read from stdin
+func (o buildOptions) contextFromStdin() bool {
+	return o.context == "-"
+}
+
 // NewBuildCommand creates a new `docker build` command
 func NewBuildCommand(dockerCli *command.DockerCli) *cobra.Command {
 	ulimits := make(map[string]*units.Ulimit)
@@ -155,6 +167,13 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 		buildBuff     io.Writer
 	)
 
+	if options.dockerfileFromStdin() {
+		if options.contextFromStdin() {
+			return errors.New("invalid argument: can't use stdin for both build context and dockerfile")
+		}
+		dockerfileCtx = dockerCli.In()
+	}
+
 	specifiedContext := options.context
 	progBuff = dockerCli.Out()
 	buildBuff = dockerCli.Out()
@@ -163,15 +182,8 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 		buildBuff = bytes.NewBuffer(nil)
 	}
 
-	if options.dockerfileName == "-" {
-		if specifiedContext == "-" {
-			return errors.New("invalid argument: can't use stdin for both build context and dockerfile")
-		}
-		dockerfileCtx = dockerCli.In()
-	}
-
 	switch {
-	case specifiedContext == "-":
+	case options.contextFromStdin():
 		buildCtx, relDockerfile, err = build.GetContextFromReader(dockerCli.In(), options.dockerfileName)
 	case isLocalDir(specifiedContext):
 		contextDir, relDockerfile, err = build.GetContextFromLocalDir(specifiedContext, options.dockerfileName)
@@ -196,12 +208,6 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 	}
 
 	if buildCtx == nil {
-		// And canonicalize dockerfile name to a platform-independent one
-		relDockerfile, err = archive.CanonicalTarNameForPath(relDockerfile)
-		if err != nil {
-			return errors.Errorf("cannot canonicalize dockerfile path %s: %v", relDockerfile, err)
-		}
-
 		f, err := os.Open(filepath.Join(contextDir, ".dockerignore"))
 		if err != nil && !os.IsNotExist(err) {
 			return err
@@ -220,6 +226,12 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 			return errors.Errorf("Error checking context: '%s'.", err)
 		}
 
+		// And canonicalize dockerfile name to a platform-independent one
+		relDockerfile, err = archive.CanonicalTarNameForPath(relDockerfile)
+		if err != nil {
+			return errors.Errorf("cannot canonicalize dockerfile path %s: %v", relDockerfile, err)
+		}
+
 		// If .dockerignore mentions .dockerignore or the Dockerfile then make
 		// sure we send both files over to the daemon because Dockerfile is,
 		// obviously, needed no matter what, and .dockerignore is needed to know
@@ -231,7 +243,7 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 		if keep, _ := fileutils.Matches(".dockerignore", excludes); keep {
 			excludes = append(excludes, "!.dockerignore")
 		}
-		if keep, _ := fileutils.Matches(relDockerfile, excludes); keep && dockerfileCtx == nil {
+		if keep, _ := fileutils.Matches(relDockerfile, excludes); keep && !options.dockerfileFromStdin() {
 			excludes = append(excludes, "!"+relDockerfile)
 		}
 

cli/command/image/build/context.go

@@ -134,15 +134,21 @@ func GetContextFromReader(r io.ReadCloser, dockerfileName string) (out io.ReadCl
 // Returns the absolute path to the temporary context directory, the relative
 // path of the dockerfile in that context directory, and a non-nil error on
 // success.
-func GetContextFromGitURL(gitURL, dockerfileName string) (absContextDir, relDockerfile string, err error) {
+func GetContextFromGitURL(gitURL, dockerfileName string) (string, string, error) {
 	if _, err := exec.LookPath("git"); err != nil {
-		return "", "", errors.Errorf("unable to find 'git': %v", err)
+		return "", "", errors.Wrapf(err, "unable to find 'git'")
 	}
-	if absContextDir, err = gitutils.Clone(gitURL); err != nil {
+	absContextDir, err := gitutils.Clone(gitURL)
-		return "", "", errors.Errorf("unable to 'git clone' to temporary context directory: %v", err)
+	if err != nil {
+		return "", "", errors.Wrapf(err, "unable to 'git clone' to temporary context directory")
 	}
 
-	return getDockerfileRelPath(absContextDir, dockerfileName)
+	absContextDir, err = resolveAndValidateContextPath(absContextDir)
+	if err != nil {
+		return "", "", err
+	}
+	relDockerfile, err := getDockerfileRelPath(absContextDir, dockerfileName)
+	return absContextDir, relDockerfile, err
 }
 
 // GetContextFromURL uses a remote URL as context for a `docker build`. The
@@ -166,8 +172,13 @@ func GetContextFromURL(out io.Writer, remoteURL, dockerfileName string) (io.Read
 // `docker build`. Returns the absolute path to the local context directory,
 // the relative path of the dockerfile in that context directory, and a non-nil
 // error on success.
-func GetContextFromLocalDir(localDir, dockerfileName string) (absContextDir, relDockerfile string, err error) {
+func GetContextFromLocalDir(localDir, dockerfileName string) (string, string, error) {
-	// When using a local context directory, when the Dockerfile is specified
+	localDir, err := resolveAndValidateContextPath(localDir)
+	if err != nil {
+		return "", "", err
+	}
+
+	// When using a local context directory, and the Dockerfile is specified
 	// with the `-f/--file` option then it is considered relative to the
 	// current directory and not the context directory.
 	if dockerfileName != "" && dockerfileName != "-" {
@@ -176,15 +187,16 @@ func GetContextFromLocalDir(localDir, dockerfileName string) (absContextDir, rel
 		}
 	}
 
-	return getDockerfileRelPath(localDir, dockerfileName)
+	relDockerfile, err := getDockerfileRelPath(localDir, dockerfileName)
+	return localDir, relDockerfile, err
 }
 
-// getDockerfileRelPath uses the given context directory for a `docker build`
+// resolveAndValidateContextPath uses the given context directory for a `docker build`
-// and returns the absolute path to the context directory, the relative path of
+// and returns the absolute path to the context directory.
-// the dockerfile in that context directory, and a non-nil error on success.
+func resolveAndValidateContextPath(givenContextDir string) (string, error) {
-func getDockerfileRelPath(givenContextDir, givenDockerfile string) (absContextDir, relDockerfile string, err error) {
+	absContextDir, err := filepath.Abs(givenContextDir)
-	if absContextDir, err = filepath.Abs(givenContextDir); err != nil {
+	if err != nil {
-		return "", "", errors.Errorf("unable to get absolute context directory of given context directory %q: %v", givenContextDir, err)
+		return "", errors.Errorf("unable to get absolute context directory of given context directory %q: %v", givenContextDir, err)
 	}
 
 	// The context dir might be a symbolic link, so follow it to the actual
@@ -197,17 +209,28 @@ func getDockerfileRelPath(givenContextDir, givenDockerfile string) (absContextDi
 	if !isUNC(absContextDir) {
 		absContextDir, err = filepath.EvalSymlinks(absContextDir)
 		if err != nil {
-			return "", "", errors.Errorf("unable to evaluate symlinks in context path: %v", err)
+			return "", errors.Errorf("unable to evaluate symlinks in context path: %v", err)
 		}
 	}
 
 	stat, err := os.Lstat(absContextDir)
 	if err != nil {
-		return "", "", errors.Errorf("unable to stat context directory %q: %v", absContextDir, err)
+		return "", errors.Errorf("unable to stat context directory %q: %v", absContextDir, err)
 	}
 
 	if !stat.IsDir() {
-		return "", "", errors.Errorf("context must be a directory: %s", absContextDir)
+		return "", errors.Errorf("context must be a directory: %s", absContextDir)
+	}
+	return absContextDir, err
+}
+
+// getDockerfileRelPath returns the dockerfile path relative to the context
+// directory
+func getDockerfileRelPath(absContextDir, givenDockerfile string) (string, error) {
+	var err error
+
+	if givenDockerfile == "-" {
+		return givenDockerfile, nil
 	}
 
 	absDockerfile := givenDockerfile
@@ -224,8 +247,6 @@ func getDockerfileRelPath(givenContextDir, givenDockerfile string) (absContextDi
 				absDockerfile = altPath
 			}
 		}
-	} else if absDockerfile == "-" {
-		absDockerfile = filepath.Join(absContextDir, DefaultDockerfileName)
 	}
 
 	// If not already an absolute path, the Dockerfile path should be joined to
@@ -240,32 +261,31 @@ func getDockerfileRelPath(givenContextDir, givenDockerfile string) (absContextDi
 	// an issue in golang. On Windows, EvalSymLinks does not work on UNC file
 	// paths (those starting with \\). This hack means that when using links
 	// on UNC paths, they will not be followed.
-	if givenDockerfile != "-" {
+	if !isUNC(absDockerfile) {
-		if !isUNC(absDockerfile) {
+		absDockerfile, err = filepath.EvalSymlinks(absDockerfile)
-			absDockerfile, err = filepath.EvalSymlinks(absDockerfile)
+		if err != nil {
-			if err != nil {
+			return "", errors.Errorf("unable to evaluate symlinks in Dockerfile path: %v", err)
-				return "", "", errors.Errorf("unable to evaluate symlinks in Dockerfile path: %v", err)
 
-			}
-		}
-
-		if _, err := os.Lstat(absDockerfile); err != nil {
-			if os.IsNotExist(err) {
-				return "", "", errors.Errorf("Cannot locate Dockerfile: %q", absDockerfile)
-			}
-			return "", "", errors.Errorf("unable to stat Dockerfile: %v", err)
 		}
 	}
 
-	if relDockerfile, err = filepath.Rel(absContextDir, absDockerfile); err != nil {
+	if _, err := os.Lstat(absDockerfile); err != nil {
-		return "", "", errors.Errorf("unable to get relative Dockerfile path: %v", err)
+		if os.IsNotExist(err) {
+			return "", errors.Errorf("Cannot locate Dockerfile: %q", absDockerfile)
+		}
+		return "", errors.Errorf("unable to stat Dockerfile: %v", err)
+	}
+
+	relDockerfile, err := filepath.Rel(absContextDir, absDockerfile)
+	if err != nil {
+		return "", errors.Errorf("unable to get relative Dockerfile path: %v", err)
 	}
 
 	if strings.HasPrefix(relDockerfile, ".."+string(filepath.Separator)) {
-		return "", "", errors.Errorf("The Dockerfile (%s) must be within the build context (%s)", givenDockerfile, givenContextDir)
+		return "", errors.Errorf("the Dockerfile (%s) must be within the build context", givenDockerfile)
 	}
 
-	return absContextDir, relDockerfile, nil
+	return relDockerfile, nil
 }
 
 // isUNC returns true if the path is UNC (one starting \\). It always returns

cli/command/image/build/context_test.go

@@ -12,6 +12,7 @@ import (
 	"testing"
 
 	"github.com/docker/docker/pkg/archive"
+	"github.com/docker/docker/pkg/testutil/assert"
 )
 
 const dockerfileContents = "FROM busybox"
@@ -45,19 +46,8 @@ func TestGetContextFromLocalDirNoDockerfile(t *testing.T) {
 	contextDir, cleanup := createTestTempDir(t, "", "builder-context-test")
 	defer cleanup()
 
-	absContextDir, relDockerfile, err := GetContextFromLocalDir(contextDir, "")
+	_, _, err := GetContextFromLocalDir(contextDir, "")
-
+	assert.Error(t, err, "Dockerfile")
-	if err == nil {
-		t.Fatalf("Error should not be nil")
-	}
-
-	if absContextDir != "" {
-		t.Fatalf("Absolute directory path should be empty, got: %s", absContextDir)
-	}
-
-	if relDockerfile != "" {
-		t.Fatalf("Relative path to Dockerfile should be empty, got: %s", relDockerfile)
-	}
 }
 
 func TestGetContextFromLocalDirNotExistingDir(t *testing.T) {
@@ -87,19 +77,8 @@ func TestGetContextFromLocalDirNotExistingDockerfile(t *testing.T) {
 
 	fakePath := filepath.Join(contextDir, "fake")
 
-	absContextDir, relDockerfile, err := GetContextFromLocalDir(contextDir, fakePath)
+	_, _, err := GetContextFromLocalDir(contextDir, fakePath)
-
+	assert.Error(t, err, "fake")
-	if err == nil {
-		t.Fatalf("Error should not be nil")
-	}
-
-	if absContextDir != "" {
-		t.Fatalf("Absolute directory path should be empty, got: %s", absContextDir)
-	}
-
-	if relDockerfile != "" {
-		t.Fatalf("Relative path to Dockerfile should be empty, got: %s", relDockerfile)
-	}
 }
 
 func TestGetContextFromLocalDirWithNoDirectory(t *testing.T) {
@@ -314,35 +293,13 @@ func TestValidateContextDirectoryWithOneFileExcludes(t *testing.T) {
 // When an error occurs, it terminates the test.
 func createTestTempDir(t *testing.T, dir, prefix string) (string, func()) {
 	path, err := ioutil.TempDir(dir, prefix)
-
+	assert.NilError(t, err)
-	if err != nil {
-		t.Fatalf("Error when creating directory %s with prefix %s: %s", dir, prefix, err)
-	}
 
 	return path, func() {
-		err = os.RemoveAll(path)
+		assert.NilError(t, os.RemoveAll(path))
-
-		if err != nil {
-			t.Fatalf("Error when removing directory %s: %s", path, err)
-		}
 	}
 }
 
-// createTestTempSubdir creates a temporary directory for testing.
-// It returns the created path but doesn't provide a cleanup function,
-// so createTestTempSubdir should be used only for creating temporary subdirectories
-// whose parent directories are properly cleaned up.
-// When an error occurs, it terminates the test.
-func createTestTempSubdir(t *testing.T, dir, prefix string) string {
-	path, err := ioutil.TempDir(dir, prefix)
-
-	if err != nil {
-		t.Fatalf("Error when creating directory %s with prefix %s: %s", dir, prefix, err)
-	}
-
-	return path
-}
-
 // createTestTempFile creates a temporary file within dir with specific contents and permissions.
 // When an error occurs, it terminates the test
 func createTestTempFile(t *testing.T, dir, filename, contents string, perm os.FileMode) string {