Merge pull request #7994 from erikh/parser_fix_volume_parsing

builder: Fix handling of VOLUME command where multiple volumes are specified in a space delimited list.

builder/dispatchers.go

@@ -314,23 +314,20 @@ func user(b *Builder, args []string, attributes map[string]bool) error {
 
 // VOLUME /foo
 //
-// Expose the volume /foo for use. Will also accept the JSON form, but either
-// way requires exactly one argument.
+// Expose the volume /foo for use. Will also accept the JSON array form.
 //
 func volume(b *Builder, args []string, attributes map[string]bool) error {
-	if len(args) != 1 {
+	if len(args) == 0 {
 		return fmt.Errorf("Volume cannot be empty")
 	}
 
-	volume := args
-
 	if b.Config.Volumes == nil {
 		b.Config.Volumes = map[string]struct{}{}
 	}
-	for _, v := range volume {
+	for _, v := range args {
 		b.Config.Volumes[v] = struct{}{}
 	}
-	if err := b.commit("", b.Config.Cmd, fmt.Sprintf("VOLUME %s", args)); err != nil {
+	if err := b.commit("", b.Config.Cmd, fmt.Sprintf("VOLUME %v", args)); err != nil {
 		return err
 	}
 	return nil

builder/parser/line_parsers.go

@@ -135,3 +135,21 @@ func parseMaybeJSON(rest string) (*Node, map[string]bool, error) {
 	node.Value = rest
 	return node, nil, nil
 }
+
+// parseMaybeJSONToList determines if the argument appears to be a JSON array. If
+// 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 {
+		return node, attrs, nil
+	}
+	if err == errDockerfileJSONNesting {
+		return nil, nil, err
+	}
+
+	return parseStringsWhitespaceDelimited(rest)
+}

builder/parser/parser.go

@@ -55,7 +55,7 @@ func init() {
 		"cmd":            parseMaybeJSON,
 		"entrypoint":     parseMaybeJSON,
 		"expose":         parseStringsWhitespaceDelimited,
-		"volume":         parseMaybeJSON,
+		"volume":         parseMaybeJSONToList,
 		"insert":         parseIgnore,
 	}
 }

builder/parser/testfiles/multiple-volumes/Dockerfile

@@ -0,0 +1,3 @@
+FROM foo
+
+VOLUME /opt/nagios/var /opt/nagios/etc /opt/nagios/libexec /var/log/apache2 /usr/share/snmp/mibs

builder/parser/testfiles/multiple-volumes/result

@@ -0,0 +1,2 @@
+(from "foo")
+(volume "/opt/nagios/var" "/opt/nagios/etc" "/opt/nagios/libexec" "/var/log/apache2" "/usr/share/snmp/mibs")

docs/sources/reference/builder.md

@@ -472,9 +472,10 @@ optional but default, you could use a `CMD` instruction:
 The `VOLUME` instruction will create a mount point with the specified name
 and mark it as holding externally mounted volumes from native host or other
 containers. The value can be a JSON array, `VOLUME ["/var/log/"]`, or a plain
-string, `VOLUME /var/log`. For more information/examples and mounting
-instructions via the Docker client, refer to [*Share Directories via Volumes*](
-/userguide/dockervolumes/#volume-def) documentation.
+string with multiple arguments, such as `VOLUME /var/log` or `VOLUME /var/log
+/var/db`.  For more information/examples and mounting instructions via the
+Docker client, refer to [*Share Directories via Volumes*](/userguide/dockervolumes/#volume-def)
+documentation.
 
 > **Note**:
 > The list is parsed a JSON array, which means that

integration-cli/docker_cli_build_test.go

@@ -728,13 +728,26 @@ func TestBuildWithVolumes(t *testing.T) {
 		result   map[string]map[string]struct{}
 		name     = "testbuildvolumes"
 		emptyMap = make(map[string]struct{})
-		expected = map[string]map[string]struct{}{"/test1": emptyMap, "/test2": emptyMap}
+		expected = map[string]map[string]struct{}{
+			"/test1":  emptyMap,
+			"/test2":  emptyMap,
+			"/test3":  emptyMap,
+			"/test4":  emptyMap,
+			"/test5":  emptyMap,
+			"/test6":  emptyMap,
+			"[/test7": emptyMap,
+			"/test8]": emptyMap,
+		}
 	)
 	defer deleteImages(name)
 	_, err := buildImage(name,
 		`FROM scratch
 		VOLUME /test1
-		VOLUME /test2`,
+		VOLUME /test2
+    VOLUME /test3 /test4
+    VOLUME ["/test5", "/test6"]
+    VOLUME [/test7 /test8]
+    `,
 		true)
 	if err != nil {
 		t.Fatal(err)