Move volume parsing to config.types module

This removes the last of the old service.ConfigError

Signed-off-by: Daniel Nephin <dnephin@docker.com>

compose/cli/command.py

@@ -14,7 +14,6 @@ from . import errors
 from . import verbose_proxy
 from .. import config
 from ..project import Project
-from ..service import ConfigError
 from .docker_client import docker_client
 from .utils import call_silently
 from .utils import get_version_info
@@ -84,16 +83,12 @@ def get_project(base_dir, config_path=None, project_name=None, verbose=False,
     config_details = config.find(base_dir, config_path)
 
     api_version = '1.21' if use_networking else None
-    try:
     return Project.from_dicts(
         get_project_name(config_details.working_dir, project_name),
         config.load(config_details),
         get_client(verbose=verbose, version=api_version),
         use_networking=use_networking,
-            network_driver=network_driver,
+        network_driver=network_driver)
-        )
-    except ConfigError as e:
-        raise errors.UserError(six.text_type(e))
 
 
 def get_project_name(working_dir, project_name=None):

compose/config/config.py

@@ -16,6 +16,7 @@ from .interpolation import interpolate_environment_variables
 from .types import parse_extra_hosts
 from .types import parse_restart_spec
 from .types import VolumeFromSpec
+from .types import VolumeSpec
 from .validation import validate_against_fields_schema
 from .validation import validate_against_service_schema
 from .validation import validate_extends_file_path
@@ -396,6 +397,10 @@ def finalize_service(service_config):
         service_dict['volumes_from'] = [
             VolumeFromSpec.parse(vf) for vf in service_dict['volumes_from']]
 
+    if 'volumes' in service_dict:
+        service_dict['volumes'] = [
+            VolumeSpec.parse(v) for v in service_dict['volumes']]
+
     if 'restart' in service_dict:
         service_dict['restart'] = parse_restart_spec(service_dict['restart'])
 

compose/config/types.py

@@ -4,9 +4,11 @@ Types for objects parsed from the configuration.
 from __future__ import absolute_import
 from __future__ import unicode_literals
 
+import os
 from collections import namedtuple
 
 from compose.config.errors import ConfigurationError
+from compose.const import IS_WINDOWS_PLATFORM
 
 
 class VolumeFromSpec(namedtuple('_VolumeFromSpec', 'source mode')):
@@ -59,3 +61,60 @@ def parse_extra_hosts(extra_hosts_config):
             host, ip = extra_hosts_line.split(':')
             extra_hosts_dict[host.strip()] = ip.strip()
         return extra_hosts_dict
+
+
+def normalize_paths_for_engine(external_path, internal_path):
+    """Windows paths, c:\my\path\shiny, need to be changed to be compatible with
+    the Engine. Volume paths are expected to be linux style /c/my/path/shiny/
+    """
+    if not IS_WINDOWS_PLATFORM:
+        return external_path, internal_path
+
+    if external_path:
+        drive, tail = os.path.splitdrive(external_path)
+
+        if drive:
+            external_path = '/' + drive.lower().rstrip(':') + tail
+
+        external_path = external_path.replace('\\', '/')
+
+    return external_path, internal_path.replace('\\', '/')
+
+
+class VolumeSpec(namedtuple('_VolumeSpec', 'external internal mode')):
+
+    @classmethod
+    def parse(cls, volume_config):
+        """Parse a volume_config path and split it into external:internal[:mode]
+        parts to be returned as a valid VolumeSpec.
+        """
+        if IS_WINDOWS_PLATFORM:
+            # relative paths in windows expand to include the drive, eg C:\
+            # so we join the first 2 parts back together to count as one
+            drive, tail = os.path.splitdrive(volume_config)
+            parts = tail.split(":")
+
+            if drive:
+                parts[0] = drive + parts[0]
+        else:
+            parts = volume_config.split(':')
+
+        if len(parts) > 3:
+            raise ConfigurationError(
+                "Volume %s has incorrect format, should be "
+                "external:internal[:mode]" % volume_config)
+
+        if len(parts) == 1:
+            external, internal = normalize_paths_for_engine(
+                None,
+                os.path.normpath(parts[0]))
+        else:
+            external, internal = normalize_paths_for_engine(
+                os.path.normpath(parts[0]),
+                os.path.normpath(parts[1]))
+
+        mode = 'rw'
+        if len(parts) == 3:
+            mode = parts[2]
+
+        return cls(external, internal, mode)

compose/service.py

@@ -2,7 +2,6 @@ from __future__ import absolute_import
 from __future__ import unicode_literals
 
 import logging
-import os
 import re
 import sys
 from collections import namedtuple
@@ -18,8 +17,8 @@ from docker.utils.ports import split_port
 from . import __version__
 from .config import DOCKER_CONFIG_KEYS
 from .config import merge_environment
+from .config.types import VolumeSpec
 from .const import DEFAULT_TIMEOUT
-from .const import IS_WINDOWS_PLATFORM
 from .const import LABEL_CONFIG_HASH
 from .const import LABEL_CONTAINER_NUMBER
 from .const import LABEL_ONE_OFF
@@ -67,11 +66,6 @@ class BuildError(Exception):
         self.reason = reason
 
 
-# TODO: remove
-class ConfigError(ValueError):
-    pass
-
-
 class NeedsBuildError(Exception):
     def __init__(self, service):
         self.service = service
@@ -81,9 +75,6 @@ class NoSuchImageError(Exception):
     pass
 
 
-VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')
-
-
 ServiceName = namedtuple('ServiceName', 'project service number')
 
 
@@ -613,8 +604,7 @@ class Service(object):
 
         if 'volumes' in container_options:
             container_options['volumes'] = dict(
-                (parse_volume_spec(v).internal, {})
+                (v.internal, {}) for v in container_options['volumes'])
-                for v in container_options['volumes'])
 
         container_options['environment'] = merge_environment(
             self.options.get('environment'),
@@ -899,11 +889,10 @@ def parse_repository_tag(repo_path):
 # Volumes
 
 
-def merge_volume_bindings(volumes_option, previous_container):
+def merge_volume_bindings(volumes, previous_container):
     """Return a list of volume bindings for a container. Container data volumes
     are replaced by those from the previous container.
     """
-    volumes = [parse_volume_spec(volume) for volume in volumes_option or []]
     volume_bindings = dict(
         build_volume_binding(volume)
         for volume in volumes
@@ -925,7 +914,7 @@ def get_container_data_volumes(container, volumes_option):
     volumes = []
     container_volumes = container.get('Volumes') or {}
     image_volumes = [
-        parse_volume_spec(volume)
+        VolumeSpec.parse(volume)
         for volume in
         container.image_config['ContainerConfig'].get('Volumes') or {}
     ]
@@ -972,56 +961,6 @@ def build_volume_binding(volume_spec):
     return volume_spec.internal, "{}:{}:{}".format(*volume_spec)
 
 
-def normalize_paths_for_engine(external_path, internal_path):
-    """Windows paths, c:\my\path\shiny, need to be changed to be compatible with
-    the Engine. Volume paths are expected to be linux style /c/my/path/shiny/
-    """
-    if not IS_WINDOWS_PLATFORM:
-        return external_path, internal_path
-
-    if external_path:
-        drive, tail = os.path.splitdrive(external_path)
-
-        if drive:
-            external_path = '/' + drive.lower().rstrip(':') + tail
-
-        external_path = external_path.replace('\\', '/')
-
-    return external_path, internal_path.replace('\\', '/')
-
-
-def parse_volume_spec(volume_config):
-    """
-    Parse a volume_config path and split it into external:internal[:mode]
-    parts to be returned as a valid VolumeSpec.
-    """
-    if IS_WINDOWS_PLATFORM:
-        # relative paths in windows expand to include the drive, eg C:\
-        # so we join the first 2 parts back together to count as one
-        drive, tail = os.path.splitdrive(volume_config)
-        parts = tail.split(":")
-
-        if drive:
-            parts[0] = drive + parts[0]
-    else:
-        parts = volume_config.split(':')
-
-    if len(parts) > 3:
-        raise ConfigError("Volume %s has incorrect format, should be "
-                          "external:internal[:mode]" % volume_config)
-
-    if len(parts) == 1:
-        external, internal = normalize_paths_for_engine(None, os.path.normpath(parts[0]))
-    else:
-        external, internal = normalize_paths_for_engine(os.path.normpath(parts[0]), os.path.normpath(parts[1]))
-
-    mode = 'rw'
-    if len(parts) == 3:
-        mode = parts[2]
-
-    return VolumeSpec(external, internal, mode)
-
-
 def build_volume_from(volume_from_spec):
     """
     volume_from can be either a service or a container. We want to return the

tests/acceptance/cli_test.py

@@ -38,7 +38,7 @@ def start_process(base_dir, options):
 def wait_on_process(proc, returncode=0):
     stdout, stderr = proc.communicate()
     if proc.returncode != returncode:
-        print(stderr)
+        print(stderr.decode('utf-8'))
         assert proc.returncode == returncode
     return ProcessResult(stdout.decode('utf-8'), stderr.decode('utf-8'))
 

tests/integration/project_test.py

@@ -4,6 +4,7 @@ from .testcases import DockerClientTestCase
 from compose.cli.docker_client import docker_client
 from compose.config import config
 from compose.config.types import VolumeFromSpec
+from compose.config.types import VolumeSpec
 from compose.const import LABEL_PROJECT
 from compose.container import Container
 from compose.project import Project
@@ -214,7 +215,7 @@ class ProjectTest(DockerClientTestCase):
 
     def test_project_up(self):
         web = self.create_service('web')
-        db = self.create_service('db', volumes=['/var/db'])
+        db = self.create_service('db', volumes=[VolumeSpec.parse('/var/db')])
         project = Project('composetest', [web, db], self.client)
         project.start()
         self.assertEqual(len(project.containers()), 0)
@@ -238,7 +239,7 @@ class ProjectTest(DockerClientTestCase):
 
     def test_recreate_preserves_volumes(self):
         web = self.create_service('web')
-        db = self.create_service('db', volumes=['/etc'])
+        db = self.create_service('db', volumes=[VolumeSpec.parse('/etc')])
         project = Project('composetest', [web, db], self.client)
         project.start()
         self.assertEqual(len(project.containers()), 0)
@@ -257,7 +258,7 @@ class ProjectTest(DockerClientTestCase):
 
     def test_project_up_with_no_recreate_running(self):
         web = self.create_service('web')
-        db = self.create_service('db', volumes=['/var/db'])
+        db = self.create_service('db', volumes=[VolumeSpec.parse('/var/db')])
         project = Project('composetest', [web, db], self.client)
         project.start()
         self.assertEqual(len(project.containers()), 0)
@@ -277,7 +278,7 @@ class ProjectTest(DockerClientTestCase):
 
     def test_project_up_with_no_recreate_stopped(self):
         web = self.create_service('web')
-        db = self.create_service('db', volumes=['/var/db'])
+        db = self.create_service('db', volumes=[VolumeSpec.parse('/var/db')])
         project = Project('composetest', [web, db], self.client)
         project.start()
         self.assertEqual(len(project.containers()), 0)
@@ -316,7 +317,7 @@ class ProjectTest(DockerClientTestCase):
 
     def test_project_up_starts_links(self):
         console = self.create_service('console')
-        db = self.create_service('db', volumes=['/var/db'])
+        db = self.create_service('db', volumes=[VolumeSpec.parse('/var/db')])
         web = self.create_service('web', links=[(db, 'db')])
 
         project = Project('composetest', [web, db, console], self.client)

tests/integration/resilience_test.py

@@ -3,13 +3,17 @@ from __future__ import unicode_literals
 
 from .. import mock
 from .testcases import DockerClientTestCase
+from compose.config.types import VolumeSpec
 from compose.project import Project
 from compose.service import ConvergenceStrategy
 
 
 class ResilienceTest(DockerClientTestCase):
     def setUp(self):
-        self.db = self.create_service('db', volumes=['/var/db'], command='top')
+        self.db = self.create_service(
+            'db',
+            volumes=[VolumeSpec.parse('/var/db')],
+            command='top')
         self.project = Project('composetest', [self.db], self.client)
 
         container = self.db.create_container()

tests/integration/service_test.py

@@ -15,6 +15,7 @@ from .testcases import DockerClientTestCase
 from .testcases import pull_busybox
 from compose import __version__
 from compose.config.types import VolumeFromSpec
+from compose.config.types import VolumeSpec
 from compose.const import LABEL_CONFIG_HASH
 from compose.const import LABEL_CONTAINER_NUMBER
 from compose.const import LABEL_ONE_OFF
@@ -114,7 +115,7 @@ class ServiceTest(DockerClientTestCase):
         self.assertEqual(container.name, 'composetest_db_run_1')
 
     def test_create_container_with_unspecified_volume(self):
-        service = self.create_service('db', volumes=['/var/db'])
+        service = self.create_service('db', volumes=[VolumeSpec.parse('/var/db')])
         container = service.create_container()
         container.start()
         self.assertIn('/var/db', container.get('Volumes'))
@@ -176,7 +177,9 @@ class ServiceTest(DockerClientTestCase):
         host_path = '/tmp/host-path'
         container_path = '/container-path'
 
-        service = self.create_service('db', volumes=['%s:%s' % (host_path, container_path)])
+        service = self.create_service(
+            'db',
+            volumes=[VolumeSpec(host_path, container_path, 'rw')])
         container = service.create_container()
         container.start()
 
@@ -189,11 +192,10 @@ class ServiceTest(DockerClientTestCase):
                         msg=("Last component differs: %s, %s" % (actual_host_path, host_path)))
 
     def test_recreate_preserves_volume_with_trailing_slash(self):
-        """
+        """When the Compose file specifies a trailing slash in the container path, make
-        When the Compose file specifies a trailing slash in the container path, make
         sure we copy the volume over when recreating.
         """
-        service = self.create_service('data', volumes=['/data/'])
+        service = self.create_service('data', volumes=[VolumeSpec.parse('/data/')])
         old_container = create_and_start_container(service)
         volume_path = old_container.get('Volumes')['/data']
 
@@ -207,7 +209,7 @@ class ServiceTest(DockerClientTestCase):
         """
         host_path = '/tmp/data'
         container_path = '/data'
-        volumes = ['{}:{}/'.format(host_path, container_path)]
+        volumes = [VolumeSpec.parse('{}:{}/'.format(host_path, container_path))]
 
         tmp_container = self.client.create_container(
             'busybox', 'true',
@@ -261,7 +263,7 @@ class ServiceTest(DockerClientTestCase):
         service = self.create_service(
             'db',
             environment={'FOO': '1'},
-            volumes=['/etc'],
+            volumes=[VolumeSpec.parse('/etc')],
             entrypoint=['top'],
             command=['-d', '1']
         )
@@ -299,7 +301,7 @@ class ServiceTest(DockerClientTestCase):
         service = self.create_service(
             'db',
             environment={'FOO': '1'},
-            volumes=['/var/db'],
+            volumes=[VolumeSpec.parse('/var/db')],
             entrypoint=['top'],
             command=['-d', '1']
         )
@@ -337,10 +339,8 @@ class ServiceTest(DockerClientTestCase):
         self.assertEqual(new_container.get('Volumes')['/data'], volume_path)
 
     def test_execute_convergence_plan_when_image_volume_masks_config(self):
-        service = Service(
+        service = self.create_service(
-            project='composetest',
+            'db',
-            name='db',
-            client=self.client,
             build='tests/fixtures/dockerfile-with-volume',
         )
 
@@ -348,7 +348,7 @@ class ServiceTest(DockerClientTestCase):
         self.assertEqual(list(old_container.get('Volumes').keys()), ['/data'])
         volume_path = old_container.get('Volumes')['/data']
 
-        service.options['volumes'] = ['/tmp:/data']
+        service.options['volumes'] = [VolumeSpec.parse('/tmp:/data')]
 
         with mock.patch('compose.service.log') as mock_log:
             new_container, = service.execute_convergence_plan(
@@ -857,22 +857,11 @@ class ServiceTest(DockerClientTestCase):
         for pair in expected.items():
             self.assertIn(pair, labels)
 
-        service.kill()
-        service.remove_stopped()
-
-        labels_list = ["%s=%s" % pair for pair in labels_dict.items()]
-
-        service = self.create_service('web', labels=labels_list)
-        labels = create_and_start_container(service).labels.items()
-        for pair in expected.items():
-            self.assertIn(pair, labels)
-
     def test_empty_labels(self):
-        labels_list = ['foo', 'bar']
+        labels_dict = {'foo': '', 'bar': ''}
-
+        service = self.create_service('web', labels=labels_dict)
-        service = self.create_service('web', labels=labels_list)
         labels = create_and_start_container(service).labels.items()
-        for name in labels_list:
+        for name in labels_dict:
             self.assertIn((name, ''), labels)
 
     def test_custom_container_name(self):

tests/integration/testcases.py

@@ -6,9 +6,7 @@ from pytest import skip
 
 from .. import unittest
 from compose.cli.docker_client import docker_client
-from compose.config.config import process_service
 from compose.config.config import resolve_environment
-from compose.config.config import ServiceConfig
 from compose.const import LABEL_PROJECT
 from compose.progress_stream import stream_output
 from compose.service import Service
@@ -41,13 +39,12 @@ class DockerClientTestCase(unittest.TestCase):
             kwargs['command'] = ["top"]
 
         service_config = ServiceConfig('.', None, name, kwargs)
-        options = process_service(service_config)
+        kwargs['environment'] = resolve_environment(service_config)
-        options['environment'] = resolve_environment(
+
-            service_config._replace(config=options))
+        labels = dict(kwargs.setdefault('labels', {}))
-        labels = options.setdefault('labels', {})
         labels['com.docker.compose.test-name'] = self.id()
 
-        return Service(name, client=self.client, project='composetest', **options)
+        return Service(name, client=self.client, project='composetest', **kwargs)
 
     def check_build(self, *args, **kwargs):
         kwargs.setdefault('rm', True)

tests/unit/config/config_test.py

@@ -11,6 +11,7 @@ import pytest
 
 from compose.config import config
 from compose.config.errors import ConfigurationError
+from compose.config.types import VolumeSpec
 from compose.const import IS_WINDOWS_PLATFORM
 from tests import mock
 from tests import unittest
@@ -147,7 +148,7 @@ class ConfigTest(unittest.TestCase):
                 'name': 'web',
                 'build': '/',
                 'links': ['db'],
-                'volumes': ['/home/user/project:/code'],
+                'volumes': [VolumeSpec.parse('/home/user/project:/code')],
             },
             {
                 'name': 'db',
@@ -211,7 +212,7 @@ class ConfigTest(unittest.TestCase):
             {
                 'name': 'web',
                 'image': 'example/web',
-                'volumes': ['/home/user/project:/code'],
+                'volumes': [VolumeSpec.parse('/home/user/project:/code')],
                 'labels': {'label': 'one'},
             },
         ]
@@ -626,14 +627,11 @@ class VolumeConfigTest(unittest.TestCase):
     @mock.patch.dict(os.environ)
     def test_volume_binding_with_environment_variable(self):
         os.environ['VOLUME_PATH'] = '/host/path'
-        d = config.load(
+        d = config.load(build_config_details(
-            build_config_details(
             {'foo': {'build': '.', 'volumes': ['${VOLUME_PATH}:/container/path']}},
             '.',
-                None,
+        ))[0]
-            )
+        self.assertEqual(d['volumes'], [VolumeSpec.parse('/host/path:/container/path')])
-        )[0]
-        self.assertEqual(d['volumes'], ['/host/path:/container/path'])
 
     @pytest.mark.skipif(IS_WINDOWS_PLATFORM, reason='posix paths')
     @mock.patch.dict(os.environ)
@@ -1031,19 +1029,21 @@ class EnvTest(unittest.TestCase):
             build_config_details(
                 {'foo': {'build': '.', 'volumes': ['$HOSTENV:$CONTAINERENV']}},
                 "tests/fixtures/env",
-                None,
             )
         )[0]
-        self.assertEqual(set(service_dict['volumes']), set(['/tmp:/host/tmp']))
+        self.assertEqual(
+            set(service_dict['volumes']),
+            set([VolumeSpec.parse('/tmp:/host/tmp')]))
 
         service_dict = config.load(
             build_config_details(
                 {'foo': {'build': '.', 'volumes': ['/opt${HOSTENV}:/opt${CONTAINERENV}']}},
                 "tests/fixtures/env",
-                None,
             )
         )[0]
-        self.assertEqual(set(service_dict['volumes']), set(['/opt/tmp:/opt/host/tmp']))
+        self.assertEqual(
+            set(service_dict['volumes']),
+            set([VolumeSpec.parse('/opt/tmp:/opt/host/tmp')]))
 
 
 def load_from_filename(filename):
@@ -1290,8 +1290,14 @@ class ExtendsTest(unittest.TestCase):
         dicts = load_from_filename('tests/fixtures/volume-path/docker-compose.yml')
 
         paths = [
-            '%s:/foo' % os.path.abspath('tests/fixtures/volume-path/common/foo'),
+            VolumeSpec(
-            '%s:/bar' % os.path.abspath('tests/fixtures/volume-path/bar'),
+                os.path.abspath('tests/fixtures/volume-path/common/foo'),
+                '/foo',
+                'rw'),
+            VolumeSpec(
+                os.path.abspath('tests/fixtures/volume-path/bar'),
+                '/bar',
+                'rw')
         ]
 
         self.assertEqual(set(dicts[0]['volumes']), set(paths))

tests/unit/config/types_test.py

@@ -1,4 +1,9 @@
+import pytest
+
+from compose.config.errors import ConfigurationError
 from compose.config.types import parse_extra_hosts
+from compose.config.types import VolumeSpec
+from compose.const import IS_WINDOWS_PLATFORM
 
 
 def test_parse_extra_hosts_list():
@@ -27,3 +32,35 @@ def test_parse_extra_hosts_dict():
         'www.example.com': '192.168.0.17',
         'api.example.com': '192.168.0.18'
     }
+
+
+class TestVolumeSpec(object):
+
+    def test_parse_volume_spec_only_one_path(self):
+        spec = VolumeSpec.parse('/the/volume')
+        assert spec == (None, '/the/volume', 'rw')
+
+    def test_parse_volume_spec_internal_and_external(self):
+        spec = VolumeSpec.parse('external:interval')
+        assert spec == ('external', 'interval', 'rw')
+
+    def test_parse_volume_spec_with_mode(self):
+        spec = VolumeSpec.parse('external:interval:ro')
+        assert spec == ('external', 'interval', 'ro')
+
+        spec = VolumeSpec.parse('external:interval:z')
+        assert spec == ('external', 'interval', 'z')
+
+    def test_parse_volume_spec_too_many_parts(self):
+        with pytest.raises(ConfigurationError) as exc:
+            VolumeSpec.parse('one:two:three:four')
+        assert 'has incorrect format' in exc.exconly()
+
+    @pytest.mark.xfail((not IS_WINDOWS_PLATFORM), reason='does not have a drive')
+    def test_parse_volume_windows_absolute_path(self):
+        windows_path = "c:\\Users\\me\\Documents\\shiny\\config:\\opt\\shiny\\config:ro"
+        assert VolumeSpec.parse(windows_path) == (
+            "/c/Users/me/Documents/shiny/config",
+            "/opt/shiny/config",
+            "ro"
+        )

tests/unit/service_test.py

@@ -2,12 +2,11 @@ from __future__ import absolute_import
 from __future__ import unicode_literals
 
 import docker
-import pytest
 
 from .. import mock
 from .. import unittest
 from compose.config.types import VolumeFromSpec
-from compose.const import IS_WINDOWS_PLATFORM
+from compose.config.types import VolumeSpec
 from compose.const import LABEL_CONFIG_HASH
 from compose.const import LABEL_ONE_OFF
 from compose.const import LABEL_PROJECT
@@ -15,7 +14,6 @@ from compose.const import LABEL_SERVICE
 from compose.container import Container
 from compose.service import build_ulimits
 from compose.service import build_volume_binding
-from compose.service import ConfigError
 from compose.service import ContainerNet
 from compose.service import get_container_data_volumes
 from compose.service import merge_volume_bindings
@@ -23,7 +21,6 @@ from compose.service import NeedsBuildError
 from compose.service import Net
 from compose.service import NoSuchImageError
 from compose.service import parse_repository_tag
-from compose.service import parse_volume_spec
 from compose.service import Service
 from compose.service import ServiceNet
 from compose.service import VolumeFromSpec
@@ -585,46 +582,12 @@ class ServiceVolumesTest(unittest.TestCase):
     def setUp(self):
         self.mock_client = mock.create_autospec(docker.Client)
 
-    def test_parse_volume_spec_only_one_path(self):
-        spec = parse_volume_spec('/the/volume')
-        self.assertEqual(spec, (None, '/the/volume', 'rw'))
-
-    def test_parse_volume_spec_internal_and_external(self):
-        spec = parse_volume_spec('external:interval')
-        self.assertEqual(spec, ('external', 'interval', 'rw'))
-
-    def test_parse_volume_spec_with_mode(self):
-        spec = parse_volume_spec('external:interval:ro')
-        self.assertEqual(spec, ('external', 'interval', 'ro'))
-
-        spec = parse_volume_spec('external:interval:z')
-        self.assertEqual(spec, ('external', 'interval', 'z'))
-
-    def test_parse_volume_spec_too_many_parts(self):
-        with self.assertRaises(ConfigError):
-            parse_volume_spec('one:two:three:four')
-
-    @pytest.mark.xfail((not IS_WINDOWS_PLATFORM), reason='does not have a drive')
-    def test_parse_volume_windows_absolute_path(self):
-        windows_absolute_path = "c:\\Users\\me\\Documents\\shiny\\config:\\opt\\shiny\\config:ro"
-
-        spec = parse_volume_spec(windows_absolute_path)
-
-        self.assertEqual(
-            spec,
-            (
-                "/c/Users/me/Documents/shiny/config",
-                "/opt/shiny/config",
-                "ro"
-            )
-        )
-
     def test_build_volume_binding(self):
-        binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
+        binding = build_volume_binding(VolumeSpec.parse('/outside:/inside'))
-        self.assertEqual(binding, ('/inside', '/outside:/inside:rw'))
+        assert binding == ('/inside', '/outside:/inside:rw')
 
     def test_get_container_data_volumes(self):
-        options = [parse_volume_spec(v) for v in [
+        options = [VolumeSpec.parse(v) for v in [
             '/host/volume:/host/volume:ro',
             '/new/volume',
             '/existing/volume',
@@ -648,19 +611,19 @@ class ServiceVolumesTest(unittest.TestCase):
         }, has_been_inspected=True)
 
         expected = [
-            parse_volume_spec('/var/lib/docker/aaaaaaaa:/existing/volume:rw'),
+            VolumeSpec.parse('/var/lib/docker/aaaaaaaa:/existing/volume:rw'),
-            parse_volume_spec('/var/lib/docker/cccccccc:/mnt/image/data:rw'),
+            VolumeSpec.parse('/var/lib/docker/cccccccc:/mnt/image/data:rw'),
         ]
 
         volumes = get_container_data_volumes(container, options)
-        self.assertEqual(sorted(volumes), sorted(expected))
+        assert sorted(volumes) == sorted(expected)
 
     def test_merge_volume_bindings(self):
         options = [
-            '/host/volume:/host/volume:ro',
+            VolumeSpec.parse('/host/volume:/host/volume:ro'),
-            '/host/rw/volume:/host/rw/volume',
+            VolumeSpec.parse('/host/rw/volume:/host/rw/volume'),
-            '/new/volume',
+            VolumeSpec.parse('/new/volume'),
-            '/existing/volume',
+            VolumeSpec.parse('/existing/volume'),
         ]
 
         self.mock_client.inspect_image.return_value = {
@@ -686,8 +649,8 @@ class ServiceVolumesTest(unittest.TestCase):
             'web',
             image='busybox',
             volumes=[
-                '/host/path:/data1',
+                VolumeSpec.parse('/host/path:/data1'),
-                '/host/path:/data2',
+                VolumeSpec.parse('/host/path:/data2'),
             ],
             client=self.mock_client,
         )
@@ -716,7 +679,7 @@ class ServiceVolumesTest(unittest.TestCase):
         service = Service(
             'web',
             image='busybox',
-            volumes=['/host/path:/data'],
+            volumes=[VolumeSpec.parse('/host/path:/data')],
             client=self.mock_client,
         )
 
@@ -784,22 +747,17 @@ class ServiceVolumesTest(unittest.TestCase):
     def test_create_with_special_volume_mode(self):
         self.mock_client.inspect_image.return_value = {'Id': 'imageid'}
 
-        create_calls = []
+        self.mock_client.create_container.return_value = {'Id': 'containerid'}
-
-        def create_container(*args, **kwargs):
-            create_calls.append((args, kwargs))
-            return {'Id': 'containerid'}
-
-        self.mock_client.create_container = create_container
-
-        volumes = ['/tmp:/foo:z']
 
+        volume = '/tmp:/foo:z'
         Service(
             'web',
             client=self.mock_client,
             image='busybox',
-            volumes=volumes,
+            volumes=[VolumeSpec.parse(volume)],
         ).create_container()
 
-        self.assertEqual(len(create_calls), 1)
+        assert self.mock_client.create_container.call_count == 1
-        self.assertEqual(self.mock_client.create_host_config.call_args[1]['binds'], volumes)
+        self.assertEqual(
+            self.mock_client.create_host_config.call_args[1]['binds'],
+            [volume])