From 08b7f30fcd050244026098673b19700485308b5a Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Tue, 5 May 2015 14:27:42 -0700 Subject: [PATCH] Fix issue where build steps are duplicated in the output This fixes an issue where the build output for the "Steps" would look like: ``` Step 1: RUN echo hi echo hi ``` instead of ``` Step 1: RUN echo hi ``` Also, I noticed that there were no checks to make sure invalid Dockerfile cmd flags were caught on cmds that didn't use cmd flags at all. They would have been caught on the cmds that had flags, but cmds that didn't bother to add a new code for flags would have just ignored them. So, I added checks to each cmd to flag it. Added testcases for issues. Signed-off-by: Doug Davis --- builder/dispatchers.go | 56 ++++++++++++++++++++++++ builder/evaluator.go | 11 ++++- integration-cli/docker_cli_build_test.go | 35 +++++++++++++++ 3 files changed, 101 insertions(+), 1 deletion(-) diff --git a/builder/dispatchers.go b/builder/dispatchers.go index 195d18305d..7d1dab6407 100644 --- a/builder/dispatchers.go +++ b/builder/dispatchers.go @@ -47,6 +47,10 @@ func env(b *Builder, args []string, attributes map[string]bool, original string) return fmt.Errorf("Bad input to ENV, too many args") } + if err := b.BuilderFlags.Parse(); err != nil { + 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 @@ -97,6 +101,10 @@ func maintainer(b *Builder, args []string, attributes map[string]bool, original return fmt.Errorf("MAINTAINER requires exactly one argument") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + b.maintainer = args[0] return b.commit("", b.Config.Cmd, fmt.Sprintf("MAINTAINER %s", b.maintainer)) } @@ -114,6 +122,10 @@ func label(b *Builder, args []string, attributes map[string]bool, original strin return fmt.Errorf("Bad input to LABEL, too many args") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + commitStr := "LABEL" if b.Config.Labels == nil { @@ -142,6 +154,10 @@ func add(b *Builder, args []string, attributes map[string]bool, original string) return fmt.Errorf("ADD requires at least two arguments") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + return b.runContextCommand(args, true, true, "ADD") } @@ -154,6 +170,10 @@ func dispatchCopy(b *Builder, args []string, attributes map[string]bool, origina return fmt.Errorf("COPY requires at least two arguments") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + return b.runContextCommand(args, false, false, "COPY") } @@ -166,6 +186,10 @@ func from(b *Builder, args []string, attributes map[string]bool, original string return fmt.Errorf("FROM requires one argument") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + name := args[0] if name == NoBaseImageSpecifier { @@ -210,6 +234,10 @@ func onbuild(b *Builder, args []string, attributes map[string]bool, original str return fmt.Errorf("ONBUILD requires at least one argument") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + triggerInstruction := strings.ToUpper(strings.TrimSpace(args[0])) switch triggerInstruction { case "ONBUILD": @@ -233,6 +261,10 @@ func workdir(b *Builder, args []string, attributes map[string]bool, original str return fmt.Errorf("WORKDIR requires exactly one argument") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + workdir := args[0] if !filepath.IsAbs(workdir) { @@ -258,6 +290,10 @@ func run(b *Builder, args []string, attributes map[string]bool, original string) return fmt.Errorf("Please provide a source image with `from` prior to run") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + args = handleJsonArgs(args, attributes) if !attributes["json"] { @@ -317,6 +353,10 @@ func run(b *Builder, args []string, attributes map[string]bool, original string) // Argument handling is the same as RUN. // func cmd(b *Builder, args []string, attributes map[string]bool, original string) error { + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + cmdSlice := handleJsonArgs(args, attributes) if !attributes["json"] { @@ -345,6 +385,10 @@ func cmd(b *Builder, args []string, attributes map[string]bool, original string) // is initialized at NewBuilder time instead of through argument parsing. // func entrypoint(b *Builder, args []string, attributes map[string]bool, original string) error { + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + parsed := handleJsonArgs(args, attributes) switch { @@ -384,6 +428,10 @@ func expose(b *Builder, args []string, attributes map[string]bool, original stri return fmt.Errorf("EXPOSE requires at least one argument") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + if b.Config.ExposedPorts == nil { b.Config.ExposedPorts = make(nat.PortSet) } @@ -428,6 +476,10 @@ func user(b *Builder, args []string, attributes map[string]bool, original string return fmt.Errorf("USER requires exactly one argument") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + b.Config.User = args[0] return b.commit("", b.Config.Cmd, fmt.Sprintf("USER %v", args)) } @@ -441,6 +493,10 @@ func volume(b *Builder, args []string, attributes map[string]bool, original stri return fmt.Errorf("VOLUME requires at least one argument") } + if err := b.BuilderFlags.Parse(); err != nil { + return err + } + if b.Config.Volumes == nil { b.Config.Volumes = map[string]struct{}{} } diff --git a/builder/evaluator.go b/builder/evaluator.go index cd8bff1cec..bdcc6b29a8 100644 --- a/builder/evaluator.go +++ b/builder/evaluator.go @@ -280,7 +280,11 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error { original := ast.Original flags := ast.Flags strs := []string{} - msg := fmt.Sprintf("Step %d : %s", stepN, original) + msg := fmt.Sprintf("Step %d : %s", stepN, strings.ToUpper(cmd)) + + if len(ast.Flags) > 0 { + msg += " " + strings.Join(ast.Flags, " ") + } if cmd == "onbuild" { if ast.Next == nil { @@ -289,6 +293,11 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error { ast = ast.Next.Children[0] strs = append(strs, ast.Value) msg += " " + ast.Value + + if len(ast.Flags) > 0 { + msg += " " + strings.Join(ast.Flags, " ") + } + } // count the number of nodes that we are going to traverse first diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 20a317246e..5247a11ec5 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -5349,3 +5349,38 @@ RUN cat /proc/self/cgroup c.Fatalf("unexpected failure when running container with --cgroup-parent option - %s\n%v", string(out), err) } } + +func (s *DockerSuite) TestBuildNoDupOutput(c *check.C) { + // Check to make sure our build output prints the Dockerfile cmd + // property - there was a bug that caused it to be duplicated on the + // Step X line + name := "testbuildnodupoutput" + + _, out, err := buildImageWithOut(name, ` + FROM busybox + RUN env`, false) + if err != nil { + c.Fatalf("Build should have worked: %q", err) + } + + exp := "\nStep 1 : RUN env\n" + if !strings.Contains(out, exp) { + c.Fatalf("Bad output\nGot:%s\n\nExpected to contain:%s\n", out, exp) + } +} + +func (s *DockerSuite) TestBuildBadCmdFlag(c *check.C) { + name := "testbuildbadcmdflag" + + _, out, err := buildImageWithOut(name, ` + FROM busybox + MAINTAINER --boo joe@example.com`, false) + if err == nil { + c.Fatal("Build should have failed") + } + + exp := `"Unknown flag: boo"` + if !strings.Contains(out, exp) { + c.Fatalf("Bad output\nGot:%s\n\nExpected to contain:%s\n", out, exp) + } +}