Resolves #260, #301 and #449

Adds ~ support and ro mode support for volumes, along with some additional validation.

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

docs/yml.md

@@ -74,14 +74,18 @@ expose:
 
 ### volumes
 
-Mount paths as volumes, optionally specifying a path on the host machine (`HOST:CONTAINER`).
+Mount paths as volumes, optionally specifying a path on the host machine
+(`HOST:CONTAINER`), or an access mode (`HOST:CONTAINER:ro`).
 
-Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
+Note for fig on OSX: Mapping local volumes is currently unsupported on
+boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
+if want to map local volumes on OSX.
 
 ```
 volumes:
  - /var/lib/mysql
  - cache/:/tmp/cache
+ - ~/configs:/etc/configs/:ro
 ```
 
 ### volumes_from

fig/service.py

@@ -1,5 +1,6 @@
 from __future__ import unicode_literals
 from __future__ import absolute_import
+from collections import namedtuple
 from .packages.docker.errors import APIError
 import logging
 import re
@@ -39,6 +40,9 @@ class ConfigError(ValueError):
     pass
 
 
+VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')
+
+
 class Service(object):
     def __init__(self, name, client=None, project='default', links=None, volumes_from=None, **options):
         if not re.match('^%s+$' % VALID_NAME_CHARS, name):
@@ -214,37 +218,22 @@ class Service(object):
             return self.start_container(container, **options)
 
     def start_container(self, container=None, intermediate_container=None, **override_options):
-        if container is None:
+        container = container or self.create_container(**override_options)
-            container = self.create_container(**override_options)
+        options = dict(self.options, **override_options)
+        ports = dict(split_port(port) for port in options.get('ports') or [])
 
-        options = self.options.copy()
+        volume_bindings = dict(
-        options.update(override_options)
+            build_volume_binding(parse_volume_spec(volume))
-
+            for volume in options.get('volumes') or []
-        port_bindings = {}
+            if ':' in volume)
-
-        if options.get('ports', None) is not None:
-            for port in options['ports']:
-                internal_port, external_port = split_port(port)
-                port_bindings[internal_port] = external_port
-
-        volume_bindings = {}
-
-        if options.get('volumes', None) is not None:
-            for volume in options['volumes']:
-                if ':' in volume:
-                    external_dir, internal_dir = volume.split(':')
-                    volume_bindings[os.path.abspath(external_dir)] = {
-                        'bind': internal_dir,
-                        'ro': False,
-                    }
 
         privileged = options.get('privileged', False)
         net = options.get('net', 'bridge')
         dns = options.get('dns', None)
 
         container.start(
-            links=self._get_links(link_to_self=override_options.get('one_off', False)),
+            links=self._get_links(link_to_self=options.get('one_off', False)),
-            port_bindings=port_bindings,
+            port_bindings=ports,
             binds=volume_bindings,
             volumes_from=self._get_volumes_from(intermediate_container),
             privileged=privileged,
@@ -338,7 +327,9 @@ class Service(object):
             container_options['ports'] = ports
 
         if 'volumes' in container_options:
-            container_options['volumes'] = dict((split_volume(v)[1], {}) for v in container_options['volumes'])
+            container_options['volumes'] = dict(
+                (parse_volume_spec(v).internal, {})
+                for v in container_options['volumes'])
 
         if 'environment' in container_options:
             if isinstance(container_options['environment'], list):
@@ -433,15 +424,30 @@ def get_container_name(container):
             return name[1:]
 
 
-def split_volume(v):
+def parse_volume_spec(volume_config):
-    """
+    parts = volume_config.split(':')
-    If v is of the format EXTERNAL:INTERNAL, returns (EXTERNAL, INTERNAL).
+    if len(parts) > 3:
-    If v is of the format INTERNAL, returns (None, INTERNAL).
+        raise ConfigError("Volume %s has incorrect format, should be "
-    """
+                          "external:internal[:mode]" % volume_config)
-    if ':' in v:
+
-        return v.split(':', 1)
+    if len(parts) == 1:
-    else:
+        return VolumeSpec(None, parts[0], 'rw')
-        return (None, v)
+
+    if len(parts) == 2:
+        parts.append('rw')
+
+    external, internal, mode = parts
+    if mode not in ('rw', 'ro'):
+        raise ConfigError("Volume %s has invalid mode (%s), should be "
+                          "one of: rw, ro." % (volume_config, mode))
+
+    return VolumeSpec(external, internal, mode)
+
+
+def build_volume_binding(volume_spec):
+    internal = {'bind': volume_spec.internal, 'ro': volume_spec.mode == 'ro'}
+    external = os.path.expanduser(volume_spec.external)
+    return os.path.abspath(os.path.expandvars(external)), internal
 
 
 def split_port(port):

tests/unit/service_test.py

@@ -1,8 +1,18 @@
 from __future__ import unicode_literals
 from __future__ import absolute_import
+import os
+
 from .. import unittest
+import mock
+
 from fig import Service
-from fig.service import ConfigError, split_port
+from fig.service import (
+    ConfigError,
+    split_port,
+    parse_volume_spec,
+    build_volume_binding,
+)
+
 
 class ServiceTest(unittest.TestCase):
     def test_name_validations(self):
@@ -82,3 +92,46 @@ class ServiceTest(unittest.TestCase):
         opts = service._get_container_create_options({})
         self.assertEqual(opts['hostname'], 'name.sub', 'hostname')
         self.assertEqual(opts['domainname'], 'domain.tld', 'domainname')
+
+
+class ServiceVolumesTest(unittest.TestCase):
+
+    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'))
+
+    def test_parse_volume_spec_too_many_parts(self):
+        with self.assertRaises(ConfigError):
+            parse_volume_spec('one:two:three:four')
+
+    def test_parse_volume_bad_mode(self):
+        with self.assertRaises(ConfigError):
+            parse_volume_spec('one:two:notrw')
+
+    def test_build_volume_binding(self):
+        binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
+        self.assertEqual(
+            binding,
+            ('/outside', dict(bind='/inside', ro=False)))
+
+    @mock.patch.dict(os.environ)
+    def test_build_volume_binding_with_environ(self):
+        os.environ['VOLUME_PATH'] = '/opt'
+        binding = build_volume_binding(parse_volume_spec('${VOLUME_PATH}:/opt'))
+        self.assertEqual(binding, ('/opt', dict(bind='/opt', ro=False)))
+
+    @mock.patch.dict(os.environ)
+    def test_building_volume_binding_with_home(self):
+        os.environ['HOME'] = '/home/user'
+        binding = build_volume_binding(parse_volume_spec('~:/home/user'))
+        self.assertEqual(
+            binding,
+            ('/home/user', dict(bind='/home/user', ro=False)))