Merge pull request #2405 from dnephin/move_all_validation_to_config

Move all validation to config

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/__init__.py

@@ -2,7 +2,6 @@
 from .config import ConfigurationError
 from .config import DOCKER_CONFIG_KEYS
 from .config import find
-from .config import get_service_name_from_net
 from .config import load
 from .config import merge_environment
 from .config import parse_environment

compose/config/config.py

@@ -1,3 +1,5 @@
+from __future__ import absolute_import
+
 import codecs
 import logging
 import operator
@@ -12,6 +14,12 @@ from .errors import CircularReference
 from .errors import ComposeFileNotFound
 from .errors import ConfigurationError
 from .interpolation import interpolate_environment_variables
+from .sort_services import get_service_name_from_net
+from .sort_services import sort_service_dicts
+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
@@ -198,16 +206,20 @@ def load(config_details):
             service_dict)
         resolver = ServiceExtendsResolver(service_config)
         service_dict = process_service(resolver.run())
+
+        # TODO: move to validate_service()
         validate_against_service_schema(service_dict, service_config.name)
         validate_paths(service_dict)
+
+        service_dict = finalize_service(service_config._replace(config=service_dict))
         service_dict['name'] = service_config.name
         return service_dict
 
     def build_services(config_file):
-        return [
+        return sort_service_dicts([
             build_service(config_file.filename, name, service_dict)
             for name, service_dict in config_file.config.items()
-        ]
+        ])
 
     def merge_services(base, override):
         all_service_names = set(base) | set(override)
@@ -353,6 +365,7 @@ def validate_ulimits(ulimit_config):
                     "than 'hard' value".format(ulimit_config))
 
 
+# TODO: rename to normalize_service
 def process_service(service_config):
     working_dir = service_config.working_dir
     service_dict = dict(service_config.config)
@@ -370,12 +383,33 @@ def process_service(service_config):
     if 'labels' in service_dict:
         service_dict['labels'] = parse_labels(service_dict['labels'])
 
+    if 'extra_hosts' in service_dict:
+        service_dict['extra_hosts'] = parse_extra_hosts(service_dict['extra_hosts'])
+
+    # TODO: move to a validate_service()
     if 'ulimits' in service_dict:
         validate_ulimits(service_dict['ulimits'])
 
     return service_dict
 
 
+def finalize_service(service_config):
+    service_dict = dict(service_config.config)
+
+    if 'volumes_from' in service_dict:
+        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'])
+
+    return service_dict
+
+
 def merge_service_dicts_from_files(base, override):
     """When merging services from multiple files we need to merge the `extends`
     field. This is not handled by `merge_service_dicts()` which is used to
@@ -606,17 +640,6 @@ def to_list(value):
         return value
 
 
-def get_service_name_from_net(net_config):
-    if not net_config:
-        return
-
-    if not net_config.startswith('container:'):
-        return
-
-    _, net_name = net_config.split(':', 1)
-    return net_name
-
-
 def load_yaml(filename):
     try:
         with open(filename, 'r') as fh:

compose/config/errors.py

@@ -6,6 +6,10 @@ class ConfigurationError(Exception):
         return self.msg
 
 
+class DependencyError(ConfigurationError):
+    pass
+
+
 class CircularReference(ConfigurationError):
     def __init__(self, trail):
         self.trail = trail

compose/config/fields_schema.json

@@ -37,22 +37,7 @@
         "domainname": {"type": "string"},
         "entrypoint": {"$ref": "#/definitions/string_or_list"},
         "env_file": {"$ref": "#/definitions/string_or_list"},
-
+        "environment": {"$ref": "#/definitions/list_or_dict"},
-        "environment": {
-          "oneOf": [
-            {
-              "type": "object",
-              "patternProperties": {
-                ".+": {
-                  "type": ["string", "number", "boolean", "null"],
-                  "format": "environment"
-                }
-              },
-              "additionalProperties": false
-            },
-            {"type": "array", "items": {"type": "string"}, "uniqueItems": true}
-          ]
-        },
 
         "expose": {
           "type": "array",
@@ -165,10 +150,18 @@
 
     "list_or_dict": {
       "oneOf": [
-        {"type": "array", "items": {"type": "string"}, "uniqueItems": true},
+        {
-        {"type": "object"}
+          "type": "object",
+          "patternProperties": {
+            ".+": {
+              "type": ["string", "number", "boolean", "null"],
+              "format": "bool-value-in-mapping"
+            }
+          },
+          "additionalProperties": false
+        },
+        {"type": "array", "items": {"type": "string"}, "uniqueItems": true}
       ]
     }
-
   }
 }

compose/config/sort_services.py

@@ -0,0 +1,55 @@
+from compose.config.errors import DependencyError
+
+
+def get_service_name_from_net(net_config):
+    if not net_config:
+        return
+
+    if not net_config.startswith('container:'):
+        return
+
+    _, net_name = net_config.split(':', 1)
+    return net_name
+
+
+def sort_service_dicts(services):
+    # Topological sort (Cormen/Tarjan algorithm).
+    unmarked = services[:]
+    temporary_marked = set()
+    sorted_services = []
+
+    def get_service_names(links):
+        return [link.split(':')[0] for link in links]
+
+    def get_service_names_from_volumes_from(volumes_from):
+        return [volume_from.source for volume_from in volumes_from]
+
+    def get_service_dependents(service_dict, services):
+        name = service_dict['name']
+        return [
+            service for service in services
+            if (name in get_service_names(service.get('links', [])) or
+                name in get_service_names_from_volumes_from(service.get('volumes_from', [])) or
+                name == get_service_name_from_net(service.get('net')))
+        ]
+
+    def visit(n):
+        if n['name'] in temporary_marked:
+            if n['name'] in get_service_names(n.get('links', [])):
+                raise DependencyError('A service can not link to itself: %s' % n['name'])
+            if n['name'] in n.get('volumes_from', []):
+                raise DependencyError('A service can not mount itself as volume: %s' % n['name'])
+            else:
+                raise DependencyError('Circular import between %s' % ' and '.join(temporary_marked))
+        if n in unmarked:
+            temporary_marked.add(n['name'])
+            for m in get_service_dependents(n, services):
+                visit(m)
+            temporary_marked.remove(n['name'])
+            unmarked.remove(n)
+            sorted_services.insert(0, n)
+
+    while unmarked:
+        visit(unmarked[-1])
+
+    return sorted_services

compose/config/types.py

@@ -0,0 +1,120 @@
+"""
+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')):
+
+    @classmethod
+    def parse(cls, volume_from_config):
+        parts = volume_from_config.split(':')
+        if len(parts) > 2:
+            raise ConfigurationError(
+                "volume_from {} has incorrect format, should be "
+                "service[:mode]".format(volume_from_config))
+
+        if len(parts) == 1:
+            source = parts[0]
+            mode = 'rw'
+        else:
+            source, mode = parts
+
+        return cls(source, mode)
+
+
+def parse_restart_spec(restart_config):
+    if not restart_config:
+        return None
+    parts = restart_config.split(':')
+    if len(parts) > 2:
+        raise ConfigurationError(
+            "Restart %s has incorrect format, should be "
+            "mode[:max_retry]" % restart_config)
+    if len(parts) == 2:
+        name, max_retry_count = parts
+    else:
+        name, = parts
+        max_retry_count = 0
+
+    return {'Name': name, 'MaximumRetryCount': int(max_retry_count)}
+
+
+def parse_extra_hosts(extra_hosts_config):
+    if not extra_hosts_config:
+        return {}
+
+    if isinstance(extra_hosts_config, dict):
+        return dict(extra_hosts_config)
+
+    if isinstance(extra_hosts_config, list):
+        extra_hosts_dict = {}
+        for extra_hosts_line in extra_hosts_config:
+            # TODO: validate string contains ':' ?
+            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/config/validation.py

@@ -49,7 +49,7 @@ def format_ports(instance):
     return True
 
 
-@FormatChecker.cls_checks(format="environment")
+@FormatChecker.cls_checks(format="bool-value-in-mapping")
 def format_boolean_in_environment(instance):
     """
     Check if there is a boolean in the environment and display a warning.
@@ -273,7 +273,7 @@ def validate_against_fields_schema(config, filename):
     _validate_against_schema(
         config,
         "fields_schema.json",
-        format_checker=["ports", "environment"],
+        format_checker=["ports", "bool-value-in-mapping"],
         filename=filename)
 
 

compose/project.py

@@ -9,7 +9,7 @@ from docker.errors import NotFound
 
 from . import parallel
 from .config import ConfigurationError
-from .config import get_service_name_from_net
+from .config.sort_services import get_service_name_from_net
 from .const import DEFAULT_TIMEOUT
 from .const import LABEL_ONE_OFF
 from .const import LABEL_PROJECT
@@ -19,61 +19,13 @@ from .legacy import check_for_legacy_containers
 from .service import ContainerNet
 from .service import ConvergenceStrategy
 from .service import Net
-from .service import parse_volume_from_spec
 from .service import Service
 from .service import ServiceNet
-from .service import VolumeFromSpec
 
 
 log = logging.getLogger(__name__)
 
 
-def sort_service_dicts(services):
-    # Topological sort (Cormen/Tarjan algorithm).
-    unmarked = services[:]
-    temporary_marked = set()
-    sorted_services = []
-
-    def get_service_names(links):
-        return [link.split(':')[0] for link in links]
-
-    def get_service_names_from_volumes_from(volumes_from):
-        return [
-            parse_volume_from_spec(volume_from).source
-            for volume_from in volumes_from
-        ]
-
-    def get_service_dependents(service_dict, services):
-        name = service_dict['name']
-        return [
-            service for service in services
-            if (name in get_service_names(service.get('links', [])) or
-                name in get_service_names_from_volumes_from(service.get('volumes_from', [])) or
-                name == get_service_name_from_net(service.get('net')))
-        ]
-
-    def visit(n):
-        if n['name'] in temporary_marked:
-            if n['name'] in get_service_names(n.get('links', [])):
-                raise DependencyError('A service can not link to itself: %s' % n['name'])
-            if n['name'] in n.get('volumes_from', []):
-                raise DependencyError('A service can not mount itself as volume: %s' % n['name'])
-            else:
-                raise DependencyError('Circular import between %s' % ' and '.join(temporary_marked))
-        if n in unmarked:
-            temporary_marked.add(n['name'])
-            for m in get_service_dependents(n, services):
-                visit(m)
-            temporary_marked.remove(n['name'])
-            unmarked.remove(n)
-            sorted_services.insert(0, n)
-
-    while unmarked:
-        visit(unmarked[-1])
-
-    return sorted_services
-
-
 class Project(object):
     """
     A collection of services.
@@ -101,7 +53,7 @@ class Project(object):
         if use_networking:
             remove_links(service_dicts)
 
-        for service_dict in sort_service_dicts(service_dicts):
+        for service_dict in service_dicts:
             links = project.get_links(service_dict)
             volumes_from = project.get_volumes_from(service_dict)
             net = project.get_net(service_dict)
@@ -192,16 +144,15 @@ class Project(object):
     def get_volumes_from(self, service_dict):
         volumes_from = []
         if 'volumes_from' in service_dict:
-            for volume_from_config in service_dict.get('volumes_from', []):
+            for volume_from_spec in service_dict.get('volumes_from', []):
-                volume_from_spec = parse_volume_from_spec(volume_from_config)
                 # Get service
                 try:
-                    service_name = self.get_service(volume_from_spec.source)
+                    service = self.get_service(volume_from_spec.source)
-                    volume_from_spec = VolumeFromSpec(service_name, volume_from_spec.mode)
+                    volume_from_spec = volume_from_spec._replace(source=service)
                 except NoSuchService:
                     try:
-                        container_name = Container.from_id(self.client, volume_from_spec.source)
+                        container = Container.from_id(self.client, volume_from_spec.source)
-                        volume_from_spec = VolumeFromSpec(container_name, volume_from_spec.mode)
+                        volume_from_spec = volume_from_spec._replace(source=container)
                     except APIError:
                         raise ConfigurationError(
                             'Service "%s" mounts volumes from "%s", which is '
@@ -410,7 +361,3 @@ class NoSuchService(Exception):
 
     def __str__(self):
         return self.msg
-
-
-class DependencyError(ConfigurationError):
-    pass

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,9 +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.validation import VALID_NAME_CHARS
+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
@@ -71,10 +69,6 @@ class BuildError(Exception):
         self.reason = reason
 
 
-class ConfigError(ValueError):
-    pass
-
-
 class NeedsBuildError(Exception):
     def __init__(self, service):
         self.service = service
@@ -84,12 +78,6 @@ class NoSuchImageError(Exception):
     pass
 
 
-VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')
-
-
-VolumeFromSpec = namedtuple('VolumeFromSpec', 'source mode')
-
-
 ServiceName = namedtuple('ServiceName', 'project service number')
 
 
@@ -122,9 +110,6 @@ class Service(object):
         net=None,
         **options
     ):
-        if not re.match('^%s+$' % VALID_NAME_CHARS, project):
-            raise ConfigError('Invalid project name "%s" - only %s are allowed' % (project, VALID_NAME_CHARS))
-
         self.name = name
         self.client = client
         self.project = project
@@ -511,7 +496,7 @@ class Service(object):
         # TODO: Implement issue #652 here
         return build_container_name(self.project, self.name, number, one_off)
 
-    # TODO: this would benefit from github.com/docker/docker/pull/11943
+    # TODO: this would benefit from github.com/docker/docker/pull/14699
     # to remove the need to inspect every container
     def _next_container_number(self, one_off=False):
         containers = filter(None, [
@@ -604,8 +589,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'),
@@ -634,58 +618,34 @@ class Service(object):
 
     def _get_container_host_config(self, override_options, one_off=False):
         options = dict(self.options, **override_options)
-        port_bindings = build_port_bindings(options.get('ports') or [])
 
-        privileged = options.get('privileged', False)
-        cap_add = options.get('cap_add', None)
-        cap_drop = options.get('cap_drop', None)
         log_config = LogConfig(
             type=options.get('log_driver', ""),
             config=options.get('log_opt', None)
         )
-        pid = options.get('pid', None)
-        security_opt = options.get('security_opt', None)
-
-        dns = options.get('dns', None)
-        if isinstance(dns, six.string_types):
-            dns = [dns]
-
-        dns_search = options.get('dns_search', None)
-        if isinstance(dns_search, six.string_types):
-            dns_search = [dns_search]
-
-        restart = parse_restart_spec(options.get('restart', None))
-
-        extra_hosts = build_extra_hosts(options.get('extra_hosts', None))
-        read_only = options.get('read_only', None)
-
-        devices = options.get('devices', None)
-        cgroup_parent = options.get('cgroup_parent', None)
-        ulimits = build_ulimits(options.get('ulimits', None))
-
         return self.client.create_host_config(
             links=self._get_links(link_to_self=one_off),
-            port_bindings=port_bindings,
+            port_bindings=build_port_bindings(options.get('ports') or []),
             binds=options.get('binds'),
             volumes_from=self._get_volumes_from(),
-            privileged=privileged,
+            privileged=options.get('privileged', False),
             network_mode=self.net.mode,
-            devices=devices,
+            devices=options.get('devices'),
-            dns=dns,
+            dns=options.get('dns'),
-            dns_search=dns_search,
+            dns_search=options.get('dns_search'),
-            restart_policy=restart,
+            restart_policy=options.get('restart'),
-            cap_add=cap_add,
+            cap_add=options.get('cap_add'),
-            cap_drop=cap_drop,
+            cap_drop=options.get('cap_drop'),
             mem_limit=options.get('mem_limit'),
             memswap_limit=options.get('memswap_limit'),
-            ulimits=ulimits,
+            ulimits=build_ulimits(options.get('ulimits')),
             log_config=log_config,
-            extra_hosts=extra_hosts,
+            extra_hosts=options.get('extra_hosts'),
-            read_only=read_only,
+            read_only=options.get('read_only'),
-            pid_mode=pid,
+            pid_mode=options.get('pid'),
-            security_opt=security_opt,
+            security_opt=options.get('security_opt'),
             ipc_mode=options.get('ipc'),
-            cgroup_parent=cgroup_parent
+            cgroup_parent=options.get('cgroup_parent'),
         )
 
     def build(self, no_cache=False, pull=False, force_rm=False):
@@ -894,11 +854,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
@@ -920,7 +879,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 {}
     ]
@@ -967,56 +926,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
@@ -1033,21 +942,6 @@ def build_volume_from(volume_from_spec):
         return ["{}:{}".format(volume_from_spec.source.id, volume_from_spec.mode)]
 
 
-def parse_volume_from_spec(volume_from_config):
-    parts = volume_from_config.split(':')
-    if len(parts) > 2:
-        raise ConfigError("Volume %s has incorrect format, should be "
-                          "external:internal[:mode]" % volume_from_config)
-
-    if len(parts) == 1:
-        source = parts[0]
-        mode = 'rw'
-    else:
-        source, mode = parts
-
-    return VolumeFromSpec(source, mode)
-
-
 # Labels
 
 
@@ -1064,24 +958,6 @@ def build_container_labels(label_options, service_labels, number, config_hash):
     return labels
 
 
-# Restart policy
-
-
-def parse_restart_spec(restart_config):
-    if not restart_config:
-        return None
-    parts = restart_config.split(':')
-    if len(parts) > 2:
-        raise ConfigError("Restart %s has incorrect format, should be "
-                          "mode[:max_retry]" % restart_config)
-    if len(parts) == 2:
-        name, max_retry_count = parts
-    else:
-        name, = parts
-        max_retry_count = 0
-
-    return {'Name': name, 'MaximumRetryCount': int(max_retry_count)}
-
 # Ulimits
 
 
@@ -1098,31 +974,3 @@ def build_ulimits(ulimit_config):
             ulimits.append(ulimit_dict)
 
     return ulimits
-
-
-# Extra hosts
-
-
-def build_extra_hosts(extra_hosts_config):
-    if not extra_hosts_config:
-        return {}
-
-    if isinstance(extra_hosts_config, list):
-        extra_hosts_dict = {}
-        for extra_hosts_line in extra_hosts_config:
-            if not isinstance(extra_hosts_line, six.string_types):
-                raise ConfigError(
-                    "extra_hosts_config \"%s\" must be either a list of strings or a string->string mapping," %
-                    extra_hosts_config
-                )
-            host, ip = extra_hosts_line.split(':')
-            extra_hosts_dict.update({host.strip(): ip.strip()})
-        extra_hosts_config = extra_hosts_dict
-
-    if isinstance(extra_hosts_config, dict):
-        return extra_hosts_config
-
-    raise ConfigError(
-        "extra_hosts_config \"%s\" must be either a list of strings or a string->string mapping," %
-        extra_hosts_config
-    )

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

@@ -3,12 +3,13 @@ from __future__ import unicode_literals
 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
 from compose.service import ConvergenceStrategy
 from compose.service import Net
-from compose.service import VolumeFromSpec
 
 
 def build_service_dicts(service_config):
@@ -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

@@ -14,6 +14,8 @@ from .. import mock
 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
@@ -21,13 +23,10 @@ from compose.const import LABEL_PROJECT
 from compose.const import LABEL_SERVICE
 from compose.const import LABEL_VERSION
 from compose.container import Container
-from compose.service import build_extra_hosts
-from compose.service import ConfigError
 from compose.service import ConvergencePlan
 from compose.service import ConvergenceStrategy
 from compose.service import Net
 from compose.service import Service
-from compose.service import VolumeFromSpec
 
 
 def create_and_start_container(service, **override_options):
@@ -122,7 +121,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'))
@@ -139,37 +138,6 @@ class ServiceTest(DockerClientTestCase):
         container.start()
         self.assertEqual(container.get('HostConfig.CpuShares'), 73)
 
-    def test_build_extra_hosts(self):
-        # string
-        self.assertRaises(ConfigError, lambda: build_extra_hosts("www.example.com: 192.168.0.17"))
-
-        # list of strings
-        self.assertEqual(build_extra_hosts(
-            ["www.example.com:192.168.0.17"]),
-            {'www.example.com': '192.168.0.17'})
-        self.assertEqual(build_extra_hosts(
-            ["www.example.com: 192.168.0.17"]),
-            {'www.example.com': '192.168.0.17'})
-        self.assertEqual(build_extra_hosts(
-            ["www.example.com: 192.168.0.17",
-             "static.example.com:192.168.0.19",
-             "api.example.com: 192.168.0.18"]),
-            {'www.example.com': '192.168.0.17',
-             'static.example.com': '192.168.0.19',
-             'api.example.com': '192.168.0.18'})
-
-        # list of dictionaries
-        self.assertRaises(ConfigError, lambda: build_extra_hosts(
-            [{'www.example.com': '192.168.0.17'},
-             {'api.example.com': '192.168.0.18'}]))
-
-        # dictionaries
-        self.assertEqual(build_extra_hosts(
-            {'www.example.com': '192.168.0.17',
-             'api.example.com': '192.168.0.18'}),
-            {'www.example.com': '192.168.0.17',
-             'api.example.com': '192.168.0.18'})
-
     def test_create_container_with_extra_hosts_list(self):
         extra_hosts = ['somehost:162.242.195.82', 'otherhost:50.31.209.229']
         service = self.create_service('db', extra_hosts=extra_hosts)
@@ -215,7 +183,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()
 
@@ -228,11 +198,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']
 
@@ -246,7 +215,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',
@@ -300,7 +269,7 @@ class ServiceTest(DockerClientTestCase):
         service = self.create_service(
             'db',
             environment={'FOO': '1'},
-            volumes=['/etc'],
+            volumes=[VolumeSpec.parse('/etc')],
             entrypoint=['top'],
             command=['-d', '1']
         )
@@ -338,7 +307,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']
         )
@@ -376,10 +345,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',
         )
 
@@ -387,7 +354,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(
@@ -786,23 +753,21 @@ class ServiceTest(DockerClientTestCase):
         container = create_and_start_container(service)
         self.assertIsNone(container.get('HostConfig.Dns'))
 
-    def test_dns_single_value(self):
-        service = self.create_service('web', dns='8.8.8.8')
-        container = create_and_start_container(service)
-        self.assertEqual(container.get('HostConfig.Dns'), ['8.8.8.8'])
-
     def test_dns_list(self):
         service = self.create_service('web', dns=['8.8.8.8', '9.9.9.9'])
         container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.Dns'), ['8.8.8.8', '9.9.9.9'])
 
     def test_restart_always_value(self):
-        service = self.create_service('web', restart='always')
+        service = self.create_service('web', restart={'Name': 'always'})
         container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.RestartPolicy.Name'), 'always')
 
     def test_restart_on_failure_value(self):
-        service = self.create_service('web', restart='on-failure:5')
+        service = self.create_service('web', restart={
+            'Name': 'on-failure',
+            'MaximumRetryCount': 5
+        })
         container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.RestartPolicy.Name'), 'on-failure')
         self.assertEqual(container.get('HostConfig.RestartPolicy.MaximumRetryCount'), 5)
@@ -817,17 +782,7 @@ class ServiceTest(DockerClientTestCase):
         container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.CapDrop'), ['SYS_ADMIN', 'NET_ADMIN'])
 
-    def test_dns_search_no_value(self):
+    def test_dns_search(self):
-        service = self.create_service('web')
-        container = create_and_start_container(service)
-        self.assertIsNone(container.get('HostConfig.DnsSearch'))
-
-    def test_dns_search_single_value(self):
-        service = self.create_service('web', dns_search='example.com')
-        container = create_and_start_container(service)
-        self.assertEqual(container.get('HostConfig.DnsSearch'), ['example.com'])
-
-    def test_dns_search_list(self):
         service = self.create_service('web', dns_search=['dc1.example.com', 'dc2.example.com'])
         container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.DnsSearch'), ['dc1.example.com', 'dc2.example.com'])
@@ -909,22 +864,11 @@ class ServiceTest(DockerClientTestCase):
         for pair in expected.items():
             self.assertIn(pair, labels)
 
-        service.kill()
-        remove_stopped(service)
-
-        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,7 +6,6 @@ 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
@@ -41,13 +40,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/cli_test.py

@@ -124,7 +124,7 @@ class CLITestCase(unittest.TestCase):
         mock_project.get_service.return_value = Service(
             'service',
             client=mock_client,
-            restart='always',
+            restart={'Name': 'always', 'MaximumRetryCount': 0},
             image='someimage')
         command.run(mock_project, {
             'SERVICE': 'service',

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
@@ -32,7 +33,7 @@ def service_sort(services):
     return sorted(services, key=itemgetter('name'))
 
 
-def build_config_details(contents, working_dir, filename):
+def build_config_details(contents, working_dir='working_dir', filename='filename.yml'):
     return config.ConfigDetails(
         working_dir,
         [config.ConfigFile(filename, contents)])
@@ -76,7 +77,7 @@ class ConfigTest(unittest.TestCase):
                 )
             )
 
-    def test_config_invalid_service_names(self):
+    def test_load_config_invalid_service_names(self):
         for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']:
             with pytest.raises(ConfigurationError) as exc:
                 config.load(build_config_details(
@@ -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'},
             },
         ]
@@ -231,6 +232,27 @@ class ConfigTest(unittest.TestCase):
         assert "service 'bogus' doesn't have any configuration" in exc.exconly()
         assert "In file 'override.yaml'" in exc.exconly()
 
+    def test_load_sorts_in_dependency_order(self):
+        config_details = build_config_details({
+            'web': {
+                'image': 'busybox:latest',
+                'links': ['db'],
+            },
+            'db': {
+                'image': 'busybox:latest',
+                'volumes_from': ['volume:ro']
+            },
+            'volume': {
+                'image': 'busybox:latest',
+                'volumes': ['/tmp'],
+            }
+        })
+        services = config.load(config_details)
+
+        assert services[0]['name'] == 'volume'
+        assert services[1]['name'] == 'db'
+        assert services[2]['name'] == 'web'
+
     def test_config_valid_service_names(self):
         for valid_name in ['_', '-', '.__.', '_what-up.', 'what_.up----', 'whatup']:
             services = config.load(
@@ -512,6 +534,29 @@ class ConfigTest(unittest.TestCase):
 
         assert 'line 3, column 32' in exc.exconly()
 
+    def test_validate_extra_hosts_invalid(self):
+        with pytest.raises(ConfigurationError) as exc:
+            config.load(build_config_details({
+                'web': {
+                    'image': 'alpine',
+                    'extra_hosts': "www.example.com: 192.168.0.17",
+                }
+            }))
+        assert "'extra_hosts' contains an invalid type" in exc.exconly()
+
+    def test_validate_extra_hosts_invalid_list(self):
+        with pytest.raises(ConfigurationError) as exc:
+            config.load(build_config_details({
+                'web': {
+                    'image': 'alpine',
+                    'extra_hosts': [
+                        {'www.example.com': '192.168.0.17'},
+                        {'api.example.com': '192.168.0.18'}
+                    ],
+                }
+            }))
+        assert "which is an invalid type" in exc.exconly()
+
 
 class InterpolationTest(unittest.TestCase):
     @mock.patch.dict(os.environ)
@@ -603,14 +648,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)
@@ -1008,19 +1050,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):
@@ -1267,8 +1311,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/sort_services_test.py

@@ -1,6 +1,7 @@
-from .. import unittest
+from compose.config.errors import DependencyError
-from compose.project import DependencyError
+from compose.config.sort_services import sort_service_dicts
-from compose.project import sort_service_dicts
+from compose.config.types import VolumeFromSpec
+from tests import unittest
 
 
 class SortServiceTest(unittest.TestCase):
@@ -73,7 +74,7 @@ class SortServiceTest(unittest.TestCase):
             },
             {
                 'name': 'parent',
-                'volumes_from': ['child']
+                'volumes_from': [VolumeFromSpec('child', 'rw')]
             },
             {
                 'links': ['parent'],
@@ -116,7 +117,7 @@ class SortServiceTest(unittest.TestCase):
             },
             {
                 'name': 'parent',
-                'volumes_from': ['child']
+                'volumes_from': [VolumeFromSpec('child', 'ro')]
             },
             {
                 'name': 'child'
@@ -141,7 +142,7 @@ class SortServiceTest(unittest.TestCase):
             },
             {
                 'name': 'two',
-                'volumes_from': ['one']
+                'volumes_from': [VolumeFromSpec('one', 'rw')]
             },
             {
                 'name': 'one'

tests/unit/config/types_test.py

@@ -0,0 +1,66 @@
+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():
+    expected = {'www.example.com': '192.168.0.17'}
+    assert parse_extra_hosts(["www.example.com:192.168.0.17"]) == expected
+
+    expected = {'www.example.com': '192.168.0.17'}
+    assert parse_extra_hosts(["www.example.com: 192.168.0.17"]) == expected
+
+    assert parse_extra_hosts([
+        "www.example.com: 192.168.0.17",
+        "static.example.com:192.168.0.19",
+        "api.example.com: 192.168.0.18"
+    ]) == {
+        'www.example.com': '192.168.0.17',
+        'static.example.com': '192.168.0.19',
+        'api.example.com': '192.168.0.18'
+    }
+
+
+def test_parse_extra_hosts_dict():
+    assert parse_extra_hosts({
+        'www.example.com': '192.168.0.17',
+        'api.example.com': '192.168.0.18'
+    }) == {
+        '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/project_test.py

@@ -4,6 +4,7 @@ import docker
 
 from .. import mock
 from .. import unittest
+from compose.config.types import VolumeFromSpec
 from compose.const import LABEL_SERVICE
 from compose.container import Container
 from compose.project import Project
@@ -33,29 +34,6 @@ class ProjectTest(unittest.TestCase):
         self.assertEqual(project.get_service('db').name, 'db')
         self.assertEqual(project.get_service('db').options['image'], 'busybox:latest')
 
-    def test_from_dict_sorts_in_dependency_order(self):
-        project = Project.from_dicts('composetest', [
-            {
-                'name': 'web',
-                'image': 'busybox:latest',
-                'links': ['db'],
-            },
-            {
-                'name': 'db',
-                'image': 'busybox:latest',
-                'volumes_from': ['volume']
-            },
-            {
-                'name': 'volume',
-                'image': 'busybox:latest',
-                'volumes': ['/tmp'],
-            }
-        ], None)
-
-        self.assertEqual(project.services[0].name, 'volume')
-        self.assertEqual(project.services[1].name, 'db')
-        self.assertEqual(project.services[2].name, 'web')
-
     def test_from_config(self):
         dicts = [
             {
@@ -167,7 +145,7 @@ class ProjectTest(unittest.TestCase):
             {
                 'name': 'test',
                 'image': 'busybox:latest',
-                'volumes_from': ['aaa']
+                'volumes_from': [VolumeFromSpec('aaa', 'rw')]
             }
         ], self.mock_client)
         self.assertEqual(project.get_service('test')._get_volumes_from(), [container_id + ":rw"])
@@ -190,17 +168,13 @@ class ProjectTest(unittest.TestCase):
             {
                 'name': 'test',
                 'image': 'busybox:latest',
-                'volumes_from': ['vol']
+                'volumes_from': [VolumeFromSpec('vol', 'rw')]
             }
         ], self.mock_client)
         self.assertEqual(project.get_service('test')._get_volumes_from(), [container_name + ":rw"])
 
-    @mock.patch.object(Service, 'containers')
+    def test_use_volumes_from_service_container(self):
-    def test_use_volumes_from_service_container(self, mock_return):
         container_ids = ['aabbccddee', '12345']
-        mock_return.return_value = [
-            mock.Mock(id=container_id, spec=Container)
-            for container_id in container_ids]
 
         project = Project.from_dicts('test', [
             {
@@ -210,10 +184,16 @@ class ProjectTest(unittest.TestCase):
             {
                 'name': 'test',
                 'image': 'busybox:latest',
-                'volumes_from': ['vol']
+                'volumes_from': [VolumeFromSpec('vol', 'rw')]
             }
         ], None)
-        self.assertEqual(project.get_service('test')._get_volumes_from(), [container_ids[0] + ':rw'])
+        with mock.patch.object(Service, 'containers') as mock_return:
+            mock_return.return_value = [
+                mock.Mock(id=container_id, spec=Container)
+                for container_id in container_ids]
+            self.assertEqual(
+                project.get_service('test')._get_volumes_from(),
+                [container_ids[0] + ':rw'])
 
     def test_net_unset(self):
         project = Project.from_dicts('test', [

tests/unit/service_test.py

@@ -2,11 +2,11 @@ from __future__ import absolute_import
 from __future__ import unicode_literals
 
 import docker
-import pytest
 
 from .. import mock
 from .. import unittest
-from compose.const import IS_WINDOWS_PLATFORM
+from compose.config.types import VolumeFromSpec
+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
@@ -14,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
@@ -22,11 +21,8 @@ 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
-from compose.service import VolumeSpec
 from compose.service import warn_on_masked_volume
 
 
@@ -35,11 +31,6 @@ class ServiceTest(unittest.TestCase):
     def setUp(self):
         self.mock_client = mock.create_autospec(docker.Client)
 
-    def test_project_validation(self):
-        self.assertRaises(ConfigError, lambda: Service(name='foo', project='>', image='foo'))
-
-        Service(name='foo', project='bar.bar__', image='foo')
-
     def test_containers(self):
         service = Service('db', self.mock_client, 'myproject', image='foo')
         self.mock_client.containers.return_value = []
@@ -589,46 +580,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',
@@ -652,19 +609,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 = {
@@ -690,8 +647,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,
         )
@@ -720,7 +677,7 @@ class ServiceVolumesTest(unittest.TestCase):
         service = Service(
             'web',
             image='busybox',
-            volumes=['/host/path:/data'],
+            volumes=[VolumeSpec.parse('/host/path:/data')],
             client=self.mock_client,
         )
 
@@ -788,22 +745,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])