Cleanup in dispatcher.env

Remove commented code blocks
Remove some duplication in comparing and restructuring env

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

builder/dockerfile/builder.go

@@ -2,7 +2,6 @@ package dockerfile
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -20,7 +19,7 @@ import (
 	"github.com/docker/docker/builder/dockerfile/parser"
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/stringid"
-	perrors "github.com/pkg/errors"
+	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 )
 
@@ -220,7 +219,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 	total := len(dockerfile.Children)
 	for _, n := range dockerfile.Children {
 		if err := b.checkDispatch(n, false); err != nil {
-			return "", perrors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
+			return "", errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
 		}
 	}
 
@@ -253,7 +252,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 	}
 
 	if b.options.Target != "" && !b.imageContexts.isCurrentTarget(b.options.Target) {
-		return "", perrors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target)
+		return "", errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target)
 	}
 
 	b.warnOnUnusedBuildArgs()
@@ -269,7 +268,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 		}
 		b.image, err = b.docker.SquashImage(b.image, fromID)
 		if err != nil {
-			return "", perrors.Wrap(err, "error squashing image")
+			return "", errors.Wrap(err, "error squashing image")
 		}
 	}
 

builder/dockerfile/dispatchers.go

@@ -16,6 +16,7 @@ import (
 	"strings"
 	"time"
 
+	"bytes"
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/api"
 	"github.com/docker/docker/api/types"
@@ -46,45 +47,22 @@ func env(b *Builder, args []string, attributes map[string]bool, original string)
 		return err
 	}
 
-	// TODO/FIXME/NOT USED
-	// Just here to show how to use the builder flags stuff within the
-	// context of a builder command. Will remove once we actually add
-	// a builder command to something!
-	/*
-		flBool1 := b.flags.AddBool("bool1", false)
-		flStr1 := b.flags.AddString("str1", "HI")
-
-		if err := b.flags.Parse(); err != nil {
-			return err
-		}
-
-		fmt.Printf("Bool1:%v\n", flBool1)
-		fmt.Printf("Str1:%v\n", flStr1)
-	*/
-
-	commitStr := "ENV"
-
-	for j := 0; j < len(args); j++ {
-		// name  ==> args[j]
-		// value ==> args[j+1]
+	commitMessage := bytes.NewBufferString("ENV")
 
+	for j := 0; j < len(args); j += 2 {
 		if len(args[j]) == 0 {
 			return errBlankCommandNames("ENV")
 		}
-		newVar := args[j] + "=" + args[j+1] + ""
-		commitStr += " " + newVar
+		name := args[j]
+		value := args[j+1]
+		newVar := name + "=" + value
+		commitMessage.WriteString(" " + newVar)
 
 		gotOne := false
 		for i, envVar := range b.runConfig.Env {
 			envParts := strings.SplitN(envVar, "=", 2)
 			compareFrom := envParts[0]
-			compareTo := args[j]
-			if runtime.GOOS == "windows" {
-				// Case insensitive environment variables on Windows
-				compareFrom = strings.ToUpper(compareFrom)
-				compareTo = strings.ToUpper(compareTo)
-			}
-			if compareFrom == compareTo {
+			if equalEnvKeys(compareFrom, name) {
 				b.runConfig.Env[i] = newVar
 				gotOne = true
 				break
@@ -93,10 +71,9 @@ func env(b *Builder, args []string, attributes map[string]bool, original string)
 		if !gotOne {
 			b.runConfig.Env = append(b.runConfig.Env, newVar)
 		}
-		j++
 	}
 
-	return b.commit("", b.runConfig.Cmd, commitStr)
+	return b.commit("", b.runConfig.Cmd, commitMessage.String())
 }
 
 // MAINTAINER some text <maybe@an.email.address>
@@ -440,6 +417,7 @@ func run(b *Builder, args []string, attributes map[string]bool, original string)
 		return err
 	}
 
+	// FIXME: this is duplicated with the defer above in this function (i think?)
 	// revert to original config environment and set the command string to
 	// have the build-time env vars in it (if any) so that future cache look-ups
 	// properly match it.

builder/dockerfile/dispatchers_test.go

@@ -133,27 +133,34 @@ func TestCommandseBlankNames(t *testing.T) {
 }
 
 func TestEnv2Variables(t *testing.T) {
-	variables := []string{"var1", "val1", "var2", "val2"}
-
-	bflags := &BFlags{}
-	config := &container.Config{}
+	b := newBuilderWithMockBackend()
+	b.disableCommit = true
 
-	b := &Builder{flags: bflags, runConfig: config, disableCommit: true}
+	args := []string{"var1", "val1", "var2", "val2"}
+	err := env(b, args, nil, "")
+	assert.NilError(t, err)
 
-	if err := env(b, variables, nil, ""); err != nil {
-		t.Fatalf("Error when executing env: %s", err.Error())
+	expected := []string{
+		fmt.Sprintf("%s=%s", args[0], args[1]),
+		fmt.Sprintf("%s=%s", args[2], args[3]),
 	}
+	assert.DeepEqual(t, b.runConfig.Env, expected)
+}
 
-	expectedVar1 := fmt.Sprintf("%s=%s", variables[0], variables[1])
-	expectedVar2 := fmt.Sprintf("%s=%s", variables[2], variables[3])
+func TestEnvValueWithExistingRunConfigEnv(t *testing.T) {
+	b := newBuilderWithMockBackend()
+	b.disableCommit = true
+	b.runConfig.Env = []string{"var1=old", "var2=fromenv"}
 
-	if b.runConfig.Env[0] != expectedVar1 {
-		t.Fatalf("Wrong env output for first variable. Got: %s. Should be: %s", b.runConfig.Env[0], expectedVar1)
-	}
+	args := []string{"var1", "val1"}
+	err := env(b, args, nil, "")
+	assert.NilError(t, err)
 
-	if b.runConfig.Env[1] != expectedVar2 {
-		t.Fatalf("Wrong env output for second variable. Got: %s, Should be: %s", b.runConfig.Env[1], expectedVar2)
+	expected := []string{
+		fmt.Sprintf("%s=%s", args[0], args[1]),
+		"var2=fromenv",
 	}
+	assert.DeepEqual(t, b.runConfig.Env, expected)
 }
 
 func TestMaintainer(t *testing.T) {

builder/dockerfile/dispatchers_unix.go

@@ -26,3 +26,9 @@ func normaliseWorkdir(current string, requested string) (string, error) {
 func errNotJSON(command, _ string) error {
 	return fmt.Errorf("%s requires the arguments to be in JSON form", command)
 }
+
+// equalEnvKeys compare two strings and returns true if they are equal. On
+// Windows this comparison is case insensitive.
+func equalEnvKeys(from, to string) bool {
+	return from == to
+}

builder/dockerfile/dispatchers_windows.go

@@ -85,3 +85,9 @@ func errNotJSON(command, original string) error {
 	}
 	return fmt.Errorf("%s requires the arguments to be in JSON form%s", command, extra)
 }
+
+// equalEnvKeys compare two strings and returns true if they are equal. On
+// Windows this comparison is case insensitive.
+func equalEnvKeys(from, to string) bool {
+	return strings.ToUpper(from) == strings.ToUpper(to)
+}

builder/dockerfile/evaluator.go

@@ -20,13 +20,13 @@
 package dockerfile
 
 import (
-	"errors"
 	"fmt"
 	"strings"
 
 	"github.com/docker/docker/builder/dockerfile/command"
 	"github.com/docker/docker/builder/dockerfile/parser"
-	runconfigopts "github.com/docker/docker/runconfig/opts"
+	"github.com/docker/docker/runconfig/opts"
+	"github.com/pkg/errors"
 )
 
 // Environment variable interpolation will happen on these statements only.
@@ -187,7 +187,7 @@ func (b *Builder) evaluateEnv(cmd string, str string, envs []string) ([]string,
 // args that are not overriden by runConfig environment variables.
 func (b *Builder) buildArgsWithoutConfigEnv() []string {
 	envs := []string{}
-	configEnv := runconfigopts.ConvertKVStringsToMap(b.runConfig.Env)
+	configEnv := b.runConfigEnvMapping()
 
 	for key, val := range b.buildArgs.GetAllAllowed() {
 		if _, ok := configEnv[key]; !ok {
@@ -197,6 +197,10 @@ func (b *Builder) buildArgsWithoutConfigEnv() []string {
 	return envs
 }
 
+func (b *Builder) runConfigEnvMapping() map[string]string {
+	return opts.ConvertKVStringsToMap(b.runConfig.Env)
+}
+
 // checkDispatch does a simple check for syntax errors of the Dockerfile.
 // Because some of the instructions can only be validated through runtime,
 // arg, env, etc., this syntax check will not be complete and could not replace
@@ -235,5 +239,5 @@ func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error {
 		return nil
 	}
 
-	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)
+	return errors.Errorf("unknown instruction: %s", upperCasedCmd)
 }

builder/dockerfile/internals.go

@@ -35,7 +35,6 @@ import (
 	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/pkg/tarsum"
 	"github.com/docker/docker/pkg/urlutil"
-	"github.com/docker/docker/runconfig/opts"
 	"github.com/pkg/errors"
 )
 
@@ -433,11 +432,7 @@ func (b *Builder) processImageFrom(img builder.Image) error {
 	// Check to see if we have a default PATH, note that windows won't
 	// have one as it's set by HCS
 	if system.DefaultPathEnv != "" {
-		// Convert the slice of strings that represent the current list
-		// of env vars into a map so we can see if PATH is already set.
-		// If it's not set then go ahead and give it our default value
-		configEnv := opts.ConvertKVStringsToMap(b.runConfig.Env)
-		if _, ok := configEnv["PATH"]; !ok {
+		if _, ok := b.runConfigEnvMapping()["PATH"]; !ok {
 			b.runConfig.Env = append(b.runConfig.Env,
 				"PATH="+system.DefaultPathEnv)
 		}

builder/dockerfile/shell_parser.go

@@ -8,7 +8,6 @@ package dockerfile
 
 import (
 	"fmt"
-	"runtime"
 	"strings"
 	"text/scanner"
 	"unicode"
@@ -296,17 +295,10 @@ func (sw *shellWord) processName() string {
 }
 
 func (sw *shellWord) getEnv(name string) string {
-	if runtime.GOOS == "windows" {
-		// Case-insensitive environment variables on Windows
-		name = strings.ToUpper(name)
-	}
 	for _, env := range sw.envs {
 		i := strings.Index(env, "=")
 		if i < 0 {
-			if runtime.GOOS == "windows" {
-				env = strings.ToUpper(env)
-			}
-			if name == env {
+			if equalEnvKeys(name, env) {
 				// Should probably never get here, but just in case treat
 				// it like "var" and "var=" are the same
 				return ""
@@ -314,10 +306,7 @@ func (sw *shellWord) getEnv(name string) string {
 			continue
 		}
 		compareName := env[:i]
-		if runtime.GOOS == "windows" {
-			compareName = strings.ToUpper(compareName)
-		}
-		if name != compareName {
+		if !equalEnvKeys(name, compareName) {
 			continue
 		}
 		return env[i+1:]

integration-cli/docker_cli_build_test.go

@@ -6156,7 +6156,7 @@ func (s *DockerSuite) TestBuildCopyFromPreviousFromWindows(c *check.C) {
 func (s *DockerSuite) TestBuildCopyFromForbidWindowsSystemPaths(c *check.C) {
 	testRequires(c, DaemonIsWindows)
 	dockerfile := `
-		FROM ` + testEnv.MinimalBaseImage() + `		
+		FROM ` + testEnv.MinimalBaseImage() + `
 		FROM ` + testEnv.MinimalBaseImage() + `
 		COPY --from=0 %s c:\\oscopy
 		`
@@ -6173,7 +6173,7 @@ func (s *DockerSuite) TestBuildCopyFromForbidWindowsSystemPaths(c *check.C) {
 func (s *DockerSuite) TestBuildCopyFromForbidWindowsRelativePaths(c *check.C) {
 	testRequires(c, DaemonIsWindows)
 	dockerfile := `
-		FROM ` + testEnv.MinimalBaseImage() + `		
+		FROM ` + testEnv.MinimalBaseImage() + `
 		FROM ` + testEnv.MinimalBaseImage() + `
 		COPY --from=0 %s c:\\oscopy
 		`
@@ -6192,7 +6192,7 @@ func (s *DockerSuite) TestBuildCopyFromWindowsIsCaseInsensitive(c *check.C) {
 	testRequires(c, DaemonIsWindows)
 	dockerfile := `
 		FROM ` + testEnv.MinimalBaseImage() + `
-		COPY foo /	
+		COPY foo /
 		FROM ` + testEnv.MinimalBaseImage() + `
 		COPY --from=0 c:\\fOo c:\\copied
 		RUN type c:\\copied
@@ -6351,23 +6351,23 @@ func (s *DockerSuite) TestBuildLineErrorOnBuild(c *check.C) {
 }
 
 // FIXME(vdemeester) should be a unit test
-func (s *DockerSuite) TestBuildLineErrorUknownInstruction(c *check.C) {
+func (s *DockerSuite) TestBuildLineErrorUnknownInstruction(c *check.C) {
 	name := "test_build_line_error_unknown_instruction"
-	buildImage(name, build.WithDockerfile(`FROM busybox
+	cli.Docker(cli.Build(name), build.WithDockerfile(`FROM busybox
   RUN echo hello world
   NOINSTRUCTION echo ba
   RUN echo hello
   ERROR
   `)).Assert(c, icmd.Expected{
 		ExitCode: 1,
-		Err:      "Dockerfile parse error line 3: Unknown instruction: NOINSTRUCTION",
+		Err:      "Dockerfile parse error line 3: unknown instruction: NOINSTRUCTION",
 	})
 }
 
 // FIXME(vdemeester) should be a unit test
 func (s *DockerSuite) TestBuildLineErrorWithEmptyLines(c *check.C) {
 	name := "test_build_line_error_with_empty_lines"
-	buildImage(name, build.WithDockerfile(`
+	cli.Docker(cli.Build(name), build.WithDockerfile(`
   FROM busybox
 
   RUN echo hello world
@@ -6377,21 +6377,21 @@ func (s *DockerSuite) TestBuildLineErrorWithEmptyLines(c *check.C) {
   CMD ["/bin/init"]
   `)).Assert(c, icmd.Expected{
 		ExitCode: 1,
-		Err:      "Dockerfile parse error line 6: Unknown instruction: NOINSTRUCTION",
+		Err:      "Dockerfile parse error line 6: unknown instruction: NOINSTRUCTION",
 	})
 }
 
 // FIXME(vdemeester) should be a unit test
 func (s *DockerSuite) TestBuildLineErrorWithComments(c *check.C) {
 	name := "test_build_line_error_with_comments"
-	buildImage(name, build.WithDockerfile(`FROM busybox
+	cli.Docker(cli.Build(name), build.WithDockerfile(`FROM busybox
   # This will print hello world
   # and then ba
   RUN echo hello world
   NOINSTRUCTION echo ba
   `)).Assert(c, icmd.Expected{
 		ExitCode: 1,
-		Err:      "Dockerfile parse error line 5: Unknown instruction: NOINSTRUCTION",
+		Err:      "Dockerfile parse error line 5: unknown instruction: NOINSTRUCTION",
 	})
 }