From c7fad9b750f8f143a22cc5a85a1dc26573025414 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 4 Apr 2017 12:28:59 -0400 Subject: [PATCH] Cleanup in dispatcher.env Remove commented code blocks Remove some duplication in comparing and restructuring env Signed-off-by: Daniel Nephin --- builder/dockerfile/builder.go | 9 +++-- builder/dockerfile/dispatchers.go | 42 ++++++----------------- builder/dockerfile/dispatchers_test.go | 37 ++++++++++++-------- builder/dockerfile/dispatchers_unix.go | 6 ++++ builder/dockerfile/dispatchers_windows.go | 6 ++++ builder/dockerfile/evaluator.go | 12 ++++--- builder/dockerfile/internals.go | 7 +--- builder/dockerfile/shell_parser.go | 15 ++------ integration-cli/docker_cli_build_test.go | 20 +++++------ 9 files changed, 69 insertions(+), 85 deletions(-) diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index b62e7e220d..03abfafcf3 100644 --- a/builder/dockerfile/builder.go +++ b/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") } } diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index fb064e5971..18fcdaf55d 100644 --- a/builder/dockerfile/dispatchers.go +++ b/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 @@ -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. diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index 77506d712a..40178b1cc7 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/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"} + b := newBuilderWithMockBackend() + b.disableCommit = true - bflags := &BFlags{} - config := &container.Config{} + args := []string{"var1", "val1", "var2", "val2"} + err := env(b, args, nil, "") + assert.NilError(t, err) - b := &Builder{flags: bflags, runConfig: config, disableCommit: true} - - 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) - } - - if b.runConfig.Env[1] != expectedVar2 { - t.Fatalf("Wrong env output for second variable. Got: %s, Should be: %s", b.runConfig.Env[1], expectedVar2) + args := []string{"var1", "val1"} + err := env(b, args, nil, "") + assert.NilError(t, err) + + expected := []string{ + fmt.Sprintf("%s=%s", args[0], args[1]), + "var2=fromenv", } + assert.DeepEqual(t, b.runConfig.Env, expected) } func TestMaintainer(t *testing.T) { diff --git a/builder/dockerfile/dispatchers_unix.go b/builder/dockerfile/dispatchers_unix.go index 29eb2fb008..62ee371dfe 100644 --- a/builder/dockerfile/dispatchers_unix.go +++ b/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 +} diff --git a/builder/dockerfile/dispatchers_windows.go b/builder/dockerfile/dispatchers_windows.go index 471232f3c7..71f7c9288f 100644 --- a/builder/dockerfile/dispatchers_windows.go +++ b/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) +} diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index d29a18c7b1..5be23c8cda 100644 --- a/builder/dockerfile/evaluator.go +++ b/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) } diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index eecd47fdea..122bb2019e 100644 --- a/builder/dockerfile/internals.go +++ b/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) } diff --git a/builder/dockerfile/shell_parser.go b/builder/dockerfile/shell_parser.go index 8e210ad6a9..7c3cf66e83 100644 --- a/builder/dockerfile/shell_parser.go +++ b/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:] diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index ea25959e44..df48bede77 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/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", }) }