Use newer default values for mounts CLI

In the API: `Writable` changed to `ReadOnly` `Populate` changed to `NoCopy` Corresponding CLI options updated to: `volume-writable` changed to `volume-readonly` `volume-populate` changed to `volume-nocopy` Signed-off-by: Brian Goff <[email protected]> (cherry picked from commit 56f3422) Signed-off-by: Tibor Vass <[email protected]>
alexmavr · Jul 8, 2016 · 843b4a9 · 843b4a9
1 parent 272e75f
commit 843b4a9
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 30 deletions.
diff --git a/api/client/service/opts.go b/api/client/service/opts.go
@@ -176,10 +176,16 @@ func (m *MountOpt) Set(value string) error {
 		}
 	}
 
+	// Set writable as the default
 	for _, field := range fields {
 		parts := strings.SplitN(field, "=", 2)
-		if len(parts) == 1 && strings.ToLower(parts[0]) == "writable" {
-			mount.Writable = true
+		if len(parts) == 1 && strings.ToLower(parts[0]) == "readonly" {
+			mount.ReadOnly = true
+			continue
+		}
+
+		if len(parts) == 1 && strings.ToLower(parts[0]) == "volume-nocopy" {
+			volumeOptions().NoCopy = true
 			continue
 		}
 
@@ -195,15 +201,16 @@ func (m *MountOpt) Set(value string) error {
 			mount.Source = value
 		case "target":
 			mount.Target = value
-		case "writable":
-			mount.Writable, err = strconv.ParseBool(value)
+		case "readonly":
+			ro, err := strconv.ParseBool(value)
 			if err != nil {
-				return fmt.Errorf("invalid value for writable: %s", value)
+				return fmt.Errorf("invalid value for readonly: %s", value)
 			}
+			mount.ReadOnly = ro
 		case "bind-propagation":
 			bindOptions().Propagation = swarm.MountPropagation(strings.ToUpper(value))
-		case "volume-populate":
-			volumeOptions().Populate, err = strconv.ParseBool(value)
+		case "volume-nocopy":
+			volumeOptions().NoCopy, err = strconv.ParseBool(value)
 			if err != nil {
 				return fmt.Errorf("invalid value for populate: %s", value)
 			}
@@ -229,6 +236,17 @@ func (m *MountOpt) Set(value string) error {
 		return fmt.Errorf("target is required")
 	}
 
+	if mount.VolumeOptions != nil && mount.Source == "" {
+		return fmt.Errorf("source is required when specifying volume-* options")
+	}
+
+	if mount.Type == swarm.MountType("BIND") && mount.VolumeOptions != nil {
+		return fmt.Errorf("cannot mix 'volume-*' options with mount type '%s'", swarm.MountTypeBind)
+	}
+	if mount.Type == swarm.MountType("VOLUME") && mount.BindOptions != nil {
+		return fmt.Errorf("cannot mix 'bind-*' options with mount type '%s'", swarm.MountTypeVolume)
+	}
+
 	m.values = append(m.values, mount)
 	return nil
 }

diff --git a/api/client/service/opts_test.go b/api/client/service/opts_test.go
@@ -111,5 +111,53 @@ func TestMountOptSetErrorInvalidField(t *testing.T) {
 
 func TestMountOptSetErrorInvalidWritable(t *testing.T) {
 	var mount MountOpt
-	assert.Error(t, mount.Set("type=VOLUME,writable=yes"), "invalid value for writable: yes")
+	assert.Error(t, mount.Set("type=VOLUME,readonly=no"), "invalid value for readonly: no")
+}
+
+func TestMountOptDefaultEnableWritable(t *testing.T) {
+	var m MountOpt
+	assert.NilError(t, m.Set("type=bind,target=/foo,source=/foo"))
+	assert.Equal(t, m.values[0].ReadOnly, false)
+
+	m = MountOpt{}
+	assert.NilError(t, m.Set("type=bind,target=/foo,source=/foo,readonly"))
+	assert.Equal(t, m.values[0].ReadOnly, true)
+
+	m = MountOpt{}
+	assert.NilError(t, m.Set("type=bind,target=/foo,source=/foo,readonly=1"))
+	assert.Equal(t, m.values[0].ReadOnly, true)
+
+	m = MountOpt{}
+	assert.NilError(t, m.Set("type=bind,target=/foo,source=/foo,readonly=0"))
+	assert.Equal(t, m.values[0].ReadOnly, false)
+}
+
+func TestMountOptVolumeNoCopy(t *testing.T) {
+	var m MountOpt
+	assert.Error(t, m.Set("type=volume,target=/foo,volume-nocopy"), "source is required")
+
+	m = MountOpt{}
+	assert.NilError(t, m.Set("type=volume,target=/foo,source=foo"))
+	assert.Equal(t, m.values[0].VolumeOptions == nil, true)
+
+	m = MountOpt{}
+	assert.NilError(t, m.Set("type=volume,target=/foo,source=foo,volume-nocopy=true"))
+	assert.Equal(t, m.values[0].VolumeOptions != nil, true)
+	assert.Equal(t, m.values[0].VolumeOptions.NoCopy, true)
+
+	m = MountOpt{}
+	assert.NilError(t, m.Set("type=volume,target=/foo,source=foo,volume-nocopy"))
+	assert.Equal(t, m.values[0].VolumeOptions != nil, true)
+	assert.Equal(t, m.values[0].VolumeOptions.NoCopy, true)
+
+	m = MountOpt{}
+	assert.NilError(t, m.Set("type=volume,target=/foo,source=foo,volume-nocopy=1"))
+	assert.Equal(t, m.values[0].VolumeOptions != nil, true)
+	assert.Equal(t, m.values[0].VolumeOptions.NoCopy, true)
+}
+
+func TestMountOptTypeConflict(t *testing.T) {
+	var m MountOpt
+	assert.Error(t, m.Set("type=bind,target=/foo,source=/foo,volume-nocopy=true"), "cannot mix")
+	assert.Error(t, m.Set("type=volume,target=/foo,source=/foo,bind-propagation=rprivate"), "cannot mix")
 }
diff --git a/daemon/cluster/convert/container.go b/daemon/cluster/convert/container.go
@@ -26,7 +26,7 @@ func containerSpecFromGRPC(c *swarmapi.ContainerSpec) types.ContainerSpec {
 			Target:   m.Target,
 			Source:   m.Source,
 			Type:     types.MountType(strings.ToLower(swarmapi.Mount_MountType_name[int32(m.Type)])),
-			Writable: m.Writable,
+			ReadOnly: m.ReadOnly,
 		}
 
 		if m.BindOptions != nil {
@@ -37,8 +37,8 @@ func containerSpecFromGRPC(c *swarmapi.ContainerSpec) types.ContainerSpec {
 
 		if m.VolumeOptions != nil {
 			mount.VolumeOptions = &types.VolumeOptions{
-				Populate: m.VolumeOptions.Populate,
-				Labels:   m.VolumeOptions.Labels,
+				NoCopy: m.VolumeOptions.NoCopy,
+				Labels: m.VolumeOptions.Labels,
 			}
 			if m.VolumeOptions.DriverConfig != nil {
 				mount.VolumeOptions.DriverConfig = &types.Driver{
@@ -77,7 +77,7 @@ func containerToGRPC(c types.ContainerSpec) (*swarmapi.ContainerSpec, error) {
 		mount := swarmapi.Mount{
 			Target:   m.Target,
 			Source:   m.Source,
-			Writable: m.Writable,
+			ReadOnly: m.ReadOnly,
 		}
 
 		if mountType, ok := swarmapi.Mount_MountType_value[strings.ToUpper(string(m.Type))]; ok {
@@ -98,8 +98,8 @@ func containerToGRPC(c types.ContainerSpec) (*swarmapi.ContainerSpec, error) {
 
 		if m.VolumeOptions != nil {
 			mount.VolumeOptions = &swarmapi.Mount_VolumeOptions{
-				Populate: m.VolumeOptions.Populate,
-				Labels:   m.VolumeOptions.Labels,
+				NoCopy: m.VolumeOptions.NoCopy,
+				Labels: m.VolumeOptions.Labels,
 			}
 			if m.VolumeOptions.DriverConfig != nil {
 				mount.VolumeOptions.DriverConfig = &swarmapi.Driver{

diff --git a/daemon/cluster/executor/container/container.go b/daemon/cluster/executor/container/container.go
@@ -163,19 +163,23 @@ func (c *containerConfig) bindMounts() []string {
 	var r []string
 
 	for _, val := range c.spec().Mounts {
-		mask := getMountMask(&val)
 		if val.Type == api.MountTypeBind || (val.Type == api.MountTypeVolume && val.Source != "") {
-			r = append(r, fmt.Sprintf("%s:%s:%s", val.Source, val.Target, mask))
+			mask := getMountMask(&val)
+			spec := fmt.Sprintf("%s:%s", val.Source, val.Target)
+			if mask != "" {
+				spec = fmt.Sprintf("%s:%s", spec, mask)
+			}
+			r = append(r, spec)
 		}
 	}
 
 	return r
 }
 
 func getMountMask(m *api.Mount) string {
-	maskOpts := []string{"ro"}
-	if m.Writable {
-		maskOpts[0] = "rw"
+	var maskOpts []string
+	if m.ReadOnly {
+		maskOpts = append(maskOpts, "ro")
 	}
 
 	if m.BindOptions != nil {
@@ -196,7 +200,7 @@ func getMountMask(m *api.Mount) string {
 	}
 
 	if m.VolumeOptions != nil {
-		if !m.VolumeOptions.Populate {
+		if m.VolumeOptions.NoCopy {
 			maskOpts = append(maskOpts, "nocopy")
 		}
 	}

diff --git a/docs/reference/api/docker_remote_api_v1.24.md b/docs/reference/api/docker_remote_api_v1.24.md
@@ -3986,11 +3986,11 @@ JSON Parameters:
             - **Target** – Container path.
             - **Source** – Mount source (e.g. a volume name, a host path).
             - **Type** – The mount type (`bind`, or `volume`).
-            - **Writable** – A boolean indicating whether the mount should be writable.
+            - **ReadOnly** – A boolean indicating whether the mount should be read-only.
             - **BindOptions** - Optional configuration for the `bind` type.
               - **Propagation** – A propagation mode with the value `[r]private`, `[r]shared`, or `[r]slave`.
             - **VolumeOptions** – Optional configuration for the `volume` type.
-                - **Populate** – A boolean indicating if volume should be
+                - **NoCopy** – A boolean indicating if volume should be
                   populated with the data from the target. (Default false)
                 - **Labels** – User-defined name and labels for the volume.
                 - **DriverConfig** – Map of driver-specific options.
@@ -4204,11 +4204,12 @@ Update the service `id`.
             - **Target** – Container path.
             - **Source** – Mount source (e.g. a volume name, a host path).
             - **Type** – The mount type (`bind`, or `volume`).
-            - **Writable** – A boolean indicating whether the mount should be writable.
+            - **ReadOnly** – A boolean indicating whether the mount should be read-only.
             - **BindOptions** - Optional configuration for the `bind` type
               - **Propagation** – A propagation mode with the value `[r]private`, `[r]shared`, or `[r]slave`.
             - **VolumeOptions** – Optional configuration for the `volume` type.
-                - **Populate** – A boolean indicating if volume should be populated with the data from the target. (Default false)
+                - **NoCopy** – A boolean indicating if volume should be
+                  populated with the data from the target. (Default false)
                 - **Labels** – User-defined name and labels for the volume.
                 - **DriverConfig** – Map of driver-specific options.
                   - **Name** - Name of the driver to use to create the volume

diff --git a/integration-cli/docker_cli_service_create_hack_test.go b/integration-cli/docker_cli_service_create_hack_test.go
@@ -41,5 +41,5 @@ func (s *DockerSwarmSuite) TestServiceCreateMountVolume(c *check.C) {
 
 	c.Assert(mounts[0].Name, checker.Equals, "foo")
 	c.Assert(mounts[0].Destination, checker.Equals, "/foo")
-	c.Assert(mounts[0].RW, checker.Equals, false)
+	c.Assert(mounts[0].RW, checker.Equals, true)
 }
diff --git a/pkg/testutil/assert/assert.go b/pkg/testutil/assert/assert.go
@@ -2,6 +2,9 @@
 package assert
 
 import (
+	"fmt"
+	"path/filepath"
+	"runtime"
 	"strings"
 )
 
@@ -15,7 +18,7 @@ type TestingT interface {
 // they are not equal.
 func Equal(t TestingT, actual, expected interface{}) {
 	if expected != actual {
-		t.Fatalf("Expected '%v' (%T) got '%v' (%T)", expected, expected, actual, actual)
+		fatal(t, fmt.Sprintf("Expected '%v' (%T) got '%v' (%T)", expected, expected, actual, actual))
 	}
 }
 
@@ -37,26 +40,31 @@ func EqualStringSlice(t TestingT, actual, expected []string) {
 // NilError asserts that the error is nil, otherwise it fails the test.
 func NilError(t TestingT, err error) {
 	if err != nil {
-		t.Fatalf("Expected no error, got: %s", err.Error())
+		fatal(t, fmt.Sprintf("Expected no error, got: %s", err.Error()))
 	}
 }
 
 // Error asserts that error is not nil, and contains the expected text,
 // otherwise it fails the test.
 func Error(t TestingT, err error, contains string) {
 	if err == nil {
-		t.Fatalf("Expected an error, but error was nil")
+		fatal(t, "Expected an error, but error was nil")
 	}
 
 	if !strings.Contains(err.Error(), contains) {
-		t.Fatalf("Expected error to contain '%s', got '%s'", contains, err.Error())
+		fatal(t, fmt.Sprintf("Expected error to contain '%s', got '%s'", contains, err.Error()))
 	}
 }
 
 // Contains asserts that the string contains a substring, otherwise it fails the
 // test.
 func Contains(t TestingT, actual, contains string) {
 	if !strings.Contains(actual, contains) {
-		t.Fatalf("Expected '%s' to contain '%s'", actual, contains)
+		fatal(t, fmt.Sprintf("Expected '%s' to contain '%s'", actual, contains))
 	}
 }
+
+func fatal(t TestingT, msg string) {
+	_, file, line, _ := runtime.Caller(2)
+	t.Fatalf("%s:%d: %s", filepath.Base(file), line, msg)
+}