From e4f02abb51534e560311b0afcfb7b586d9587e67 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Wed, 4 Feb 2015 09:34:25 -0800 Subject: [PATCH] Add support for no-arg commands in Dockerfile We're hoping to add some new commands that don't have any args so this PR will enable that by removing all of the hard-coded checks that require commands to have at least one arg. It also adds some checks to each command so we're consistent in the error message we get. Added a test for this too. We actually had this check in at least 3 different places (twice in the parser and once in most cmds), this removes 2 of them (the parser ones). Had to remove/modify some testcases because its now legal to have certain commands w/o args - e.g. RUN. This was actually inconsistent because we used to allow "RUN []" but not "RUN" even though they would generate (almost) the same net result. Now we're consistent. Signed-off-by: Doug Davis --- builder/dispatchers.go | 14 +++-- builder/evaluator.go | 3 ++ builder/parser/line_parsers.go | 19 +++++-- builder/parser/parser.go | 5 -- .../empty-instruction/Dockerfile | 8 --- .../Dockerfile | 2 - builder/parser/utils.go | 11 ++-- integration-cli/docker_cli_build_test.go | 52 ++++++++++++++++++- 8 files changed, 85 insertions(+), 29 deletions(-) delete mode 100644 builder/parser/testfiles-negative/empty-instruction/Dockerfile delete mode 100644 builder/parser/testfiles-negative/html-page-yes-really-thanks-lk4d4/Dockerfile diff --git a/builder/dispatchers.go b/builder/dispatchers.go index 2bc56e46ee..b00268ed67 100644 --- a/builder/dispatchers.go +++ b/builder/dispatchers.go @@ -39,7 +39,7 @@ func nullDispatch(b *Builder, args []string, attributes map[string]bool, origina // func env(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) == 0 { - return fmt.Errorf("ENV is missing arguments") + return fmt.Errorf("ENV requires at least one argument") } if len(args)%2 != 0 { @@ -78,7 +78,7 @@ func env(b *Builder, args []string, attributes map[string]bool, original string) // Sets the maintainer metadata. func maintainer(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) != 1 { - return fmt.Errorf("MAINTAINER requires only one argument") + return fmt.Errorf("MAINTAINER requires exactly one argument") } b.maintainer = args[0] @@ -159,6 +159,10 @@ func from(b *Builder, args []string, attributes map[string]bool, original string // cases. // func onbuild(b *Builder, args []string, attributes map[string]bool, original string) error { + if len(args) == 0 { + return fmt.Errorf("ONBUILD requires at least one argument") + } + triggerInstruction := strings.ToUpper(strings.TrimSpace(args[0])) switch triggerInstruction { case "ONBUILD": @@ -327,6 +331,10 @@ func entrypoint(b *Builder, args []string, attributes map[string]bool, original func expose(b *Builder, args []string, attributes map[string]bool, original string) error { portsTab := args + if len(args) == 0 { + return fmt.Errorf("EXPOSE requires at least one argument") + } + if b.Config.ExposedPorts == nil { b.Config.ExposedPorts = make(nat.PortSet) } @@ -373,7 +381,7 @@ func user(b *Builder, args []string, attributes map[string]bool, original string // func volume(b *Builder, args []string, attributes map[string]bool, original string) error { if len(args) == 0 { - return fmt.Errorf("Volume cannot be empty") + return fmt.Errorf("VOLUME requires at least one argument") } if b.Config.Volumes == nil { diff --git a/builder/evaluator.go b/builder/evaluator.go index b76c7f29bb..3a588fb3ee 100644 --- a/builder/evaluator.go +++ b/builder/evaluator.go @@ -240,6 +240,9 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error { msg := fmt.Sprintf("Step %d : %s", stepN, strings.ToUpper(cmd)) if cmd == "onbuild" { + if ast.Next == nil { + return fmt.Errorf("ONBUILD requires at least one argument") + } ast = ast.Next.Children[0] strs = append(strs, ast.Value) msg += " " + ast.Value diff --git a/builder/parser/line_parsers.go b/builder/parser/line_parsers.go index 8a94e1e5db..c7fed13dbe 100644 --- a/builder/parser/line_parsers.go +++ b/builder/parser/line_parsers.go @@ -30,6 +30,10 @@ func parseIgnore(rest string) (*Node, map[string]bool, error) { // ONBUILD RUN foo bar -> (onbuild (run foo bar)) // func parseSubCommand(rest string) (*Node, map[string]bool, error) { + if rest == "" { + return nil, nil, nil + } + _, child, err := parseLine(rest) if err != nil { return nil, nil, err @@ -133,7 +137,7 @@ func parseEnv(rest string) (*Node, map[string]bool, error) { } if len(words) == 0 { - return nil, nil, fmt.Errorf("ENV must have some arguments") + return nil, nil, fmt.Errorf("ENV requires at least one argument") } // Old format (ENV name value) @@ -181,6 +185,10 @@ func parseEnv(rest string) (*Node, map[string]bool, error) { // parses a whitespace-delimited set of arguments. The result is effectively a // linked list of string arguments. func parseStringsWhitespaceDelimited(rest string) (*Node, map[string]bool, error) { + if rest == "" { + return nil, nil, nil + } + node := &Node{} rootnode := node prevnode := node @@ -201,6 +209,9 @@ func parseStringsWhitespaceDelimited(rest string) (*Node, map[string]bool, error // parsestring just wraps the string in quotes and returns a working node. func parseString(rest string) (*Node, map[string]bool, error) { + if rest == "" { + return nil, nil, nil + } n := &Node{} n.Value = rest return n, nil, nil @@ -235,7 +246,9 @@ func parseJSON(rest string) (*Node, map[string]bool, error) { // so, passes to parseJSON; if not, quotes the result and returns a single // node. func parseMaybeJSON(rest string) (*Node, map[string]bool, error) { - rest = strings.TrimSpace(rest) + if rest == "" { + return nil, nil, nil + } node, attrs, err := parseJSON(rest) @@ -255,8 +268,6 @@ func parseMaybeJSON(rest string) (*Node, map[string]bool, error) { // so, passes to parseJSON; if not, attmpts to parse it as a whitespace // delimited string. func parseMaybeJSONToList(rest string) (*Node, map[string]bool, error) { - rest = strings.TrimSpace(rest) - node, attrs, err := parseJSON(rest) if err == nil { diff --git a/builder/parser/parser.go b/builder/parser/parser.go index 957ecd6294..b8685767b9 100644 --- a/builder/parser/parser.go +++ b/builder/parser/parser.go @@ -3,7 +3,6 @@ package parser import ( "bufio" - "fmt" "io" "regexp" "strings" @@ -78,10 +77,6 @@ func parseLine(line string) (string, *Node, error) { return "", nil, err } - if len(args) == 0 { - return "", nil, fmt.Errorf("Instruction %q is empty; cannot continue", cmd) - } - node := &Node{} node.Value = cmd diff --git a/builder/parser/testfiles-negative/empty-instruction/Dockerfile b/builder/parser/testfiles-negative/empty-instruction/Dockerfile deleted file mode 100644 index 74e625a402..0000000000 --- a/builder/parser/testfiles-negative/empty-instruction/Dockerfile +++ /dev/null @@ -1,8 +0,0 @@ -FROM dockerfile/rabbitmq - -RUN - rabbitmq-plugins enable \ - rabbitmq_shovel \ - rabbitmq_shovel_management \ - rabbitmq_federation \ - rabbitmq_federation_management diff --git a/builder/parser/testfiles-negative/html-page-yes-really-thanks-lk4d4/Dockerfile b/builder/parser/testfiles-negative/html-page-yes-really-thanks-lk4d4/Dockerfile deleted file mode 100644 index 90531a4b3e..0000000000 --- a/builder/parser/testfiles-negative/html-page-yes-really-thanks-lk4d4/Dockerfile +++ /dev/null @@ -1,2 +0,0 @@ - - diff --git a/builder/parser/utils.go b/builder/parser/utils.go index b64f67f118..a60ad129fe 100644 --- a/builder/parser/utils.go +++ b/builder/parser/utils.go @@ -1,7 +1,6 @@ package parser import ( - "fmt" "strconv" "strings" ) @@ -50,17 +49,19 @@ func fullDispatch(cmd, args string) (*Node, map[string]bool, error) { // splitCommand takes a single line of text and parses out the cmd and args, // which are used for dispatching to more exact parsing functions. func splitCommand(line string) (string, string, error) { + var args string + // Make sure we get the same results irrespective of leading/trailing spaces cmdline := TOKEN_WHITESPACE.Split(strings.TrimSpace(line), 2) + cmd := strings.ToLower(cmdline[0]) - if len(cmdline) != 2 { - return "", "", fmt.Errorf("We do not understand this file. Please ensure it is a valid Dockerfile. Parser error at %q", line) + if len(cmdline) == 2 { + args = strings.TrimSpace(cmdline[1]) } - cmd := strings.ToLower(cmdline[0]) // the cmd should never have whitespace, but it's possible for the args to // have trailing whitespace. - return cmd, strings.TrimSpace(cmdline[1]), nil + return cmd, args, nil } // covers comments and empty lines. Lines should be trimmed before passing to diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index a692a2b87d..f26512ec6f 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -49,14 +49,14 @@ func TestBuildEmptyWhitespace(t *testing.T) { name, ` FROM busybox - RUN + COPY quux \ bar `, true) if err == nil { - t.Fatal("no error when dealing with a RUN statement with no content on the same line") + t.Fatal("no error when dealing with a COPY statement with no content on the same line") } logDone("build - statements with whitespace and no content should generate a parse error") @@ -4822,3 +4822,51 @@ func TestBuildVolumeFileExistsinContainer(t *testing.T) { logDone("build - errors when volume is specified where a file exists") } + +func TestBuildMissingArgs(t *testing.T) { + // test to make sure these cmds w/o any args will generate an error + // Notice some commands are missing because its ok for them to + // not have any args - like: CMD, RUN, ENTRYPOINT + cmds := []string{ + "ADD", + "COPY", + "ENV", + "EXPOSE", + "FROM", + "MAINTAINER", + "ONBUILD", + "USER", + "VOLUME", + "WORKDIR", + } + + defer deleteAllContainers() + + for _, cmd := range cmds { + var dockerfile string + + if cmd == "FROM" { + dockerfile = cmd + } else { + // Add FROM to make sure we don't complain about it missing + dockerfile = "FROM busybox\n" + cmd + } + + ctx, err := fakeContext(dockerfile, map[string]string{}) + if err != nil { + t.Fatal(err) + } + defer ctx.Close() + var out string + if out, err = buildImageFromContext("args", ctx, true); err == nil { + t.Fatalf("%s was supposed to fail:%s", cmd, out) + } + if !strings.Contains(err.Error(), cmd+" requires") { + t.Fatalf("%s returned the wrong type of error:%s", cmd, err) + } + + ctx.Close() + } + + logDone("build - verify missing args") +}