Merge pull request #1384 from docker/dzimine-dz-fix-mount_options

Fix parse_mount_string

3 files changed, 44 insertions, 9 deletions

diff --git a/docker/errors.py b/docker/errors.py
index 05f4cae..95c462b 100644
--- a/docker/errors.py
+++ b/docker/errors.py
@@ -93,6 +93,10 @@ class InvalidConfigFile(DockerException):
     pass
 
 
+class InvalidArgument(DockerException):
+    pass
+
+
 class DeprecatedMethod(DockerException):
     pass
 
diff --git a/docker/types/services.py b/docker/types/services.py
index 5041f89..b52afd2 100644
--- a/docker/types/services.py
+++ b/docker/types/services.py
@@ -131,10 +131,11 @@ class Mount(dict):
         self['Target'] = target
         self['Source'] = source
         if type not in ('bind', 'volume'):
-            raise errors.DockerError(
+            raise errors.InvalidArgument(
                 'Only acceptable mount types are `bind` and `volume`.'
             )
         self['Type'] = type
+        self['ReadOnly'] = read_only
 
         if type == 'bind':
             if propagation is not None:
@@ -142,7 +143,7 @@ class Mount(dict):
                     'Propagation': propagation
                 }
             if any([labels, driver_config, no_copy]):
-                raise errors.DockerError(
+                raise errors.InvalidArgument(
                     'Mount type is binding but volume options have been '
                     'provided.'
                 )
@@ -157,7 +158,7 @@ class Mount(dict):
             if volume_opts:
                 self['VolumeOptions'] = volume_opts
             if propagation:
-                raise errors.DockerError(
+                raise errors.InvalidArgument(
                     'Mount type is volume but `propagation` argument has been '
                     'provided.'
                 )
@@ -166,15 +167,15 @@ class Mount(dict):
     def parse_mount_string(cls, string):
         parts = string.split(':')
         if len(parts) > 3:
-            raise errors.DockerError(
+            raise errors.InvalidArgument(
                 'Invalid mount format "{0}"'.format(string)
             )
         if len(parts) == 1:
-            return cls(target=parts[0])
+            return cls(target=parts[0], source=None)
         else:
             target = parts[1]
             source = parts[0]
-            read_only = not (len(parts) == 3 or parts[2] == 'ro')
+            read_only = not (len(parts) == 2 or parts[2] == 'rw')
             return cls(target, source, read_only=read_only)
 
 
@@ -228,7 +229,7 @@ class UpdateConfig(dict):
         if delay is not None:
             self['Delay'] = delay
         if failure_action not in ('pause', 'continue'):
-            raise errors.DockerError(
+            raise errors.InvalidArgument(
                 'failure_action must be either `pause` or `continue`.'
             )
         self['FailureAction'] = failure_action
diff --git a/tests/unit/dockertypes_test.py b/tests/unit/dockertypes_test.py
index 2480b9e..5cf5f4e 100644
--- a/tests/unit/dockertypes_test.py
+++ b/tests/unit/dockertypes_test.py
@@ -5,9 +5,9 @@ import unittest
 import pytest
 
 from docker.constants import DEFAULT_DOCKER_API_VERSION
-from docker.errors import InvalidVersion
+from docker.errors import InvalidArgument, InvalidVersion
 from docker.types import (
-    EndpointConfig, HostConfig, IPAMConfig, IPAMPool, LogConfig, Ulimit,
+    EndpointConfig, HostConfig, IPAMConfig, IPAMPool, LogConfig, Mount, Ulimit,
 )
 
 
@@ -253,3 +253,33 @@ class IPAMConfigTest(unittest.TestCase):
                 'IPRange': None,
             }]
         })
+
+
+class TestMounts(unittest.TestCase):
+    def test_parse_mount_string_ro(self):
+        mount = Mount.parse_mount_string("/foo/bar:/baz:ro")
+        self.assertEqual(mount['Source'], "/foo/bar")
+        self.assertEqual(mount['Target'], "/baz")
+        self.assertEqual(mount['ReadOnly'], True)
+
+    def test_parse_mount_string_rw(self):
+        mount = Mount.parse_mount_string("/foo/bar:/baz:rw")
+        self.assertEqual(mount['Source'], "/foo/bar")
+        self.assertEqual(mount['Target'], "/baz")
+        self.assertEqual(mount['ReadOnly'], False)
+
+    def test_parse_mount_string_short_form(self):
+        mount = Mount.parse_mount_string("/foo/bar:/baz")
+        self.assertEqual(mount['Source'], "/foo/bar")
+        self.assertEqual(mount['Target'], "/baz")
+        self.assertEqual(mount['ReadOnly'], False)
+
+    def test_parse_mount_string_no_source(self):
+        mount = Mount.parse_mount_string("foo/bar")
+        self.assertEqual(mount['Source'], None)
+        self.assertEqual(mount['Target'], "foo/bar")
+        self.assertEqual(mount['ReadOnly'], False)
+
+    def test_parse_mount_string_invalid(self):
+        with pytest.raises(InvalidArgument):
+            Mount.parse_mount_string("foo:bar:baz:rw")