Repository: JonathonReinhart/scuba

Issue:
Allow Docker Arguments in Scubafile
Currently Scuba has the `-d` or `--docker-args` flag that allows the user to specify arguments that should be passed-through to docker. This is an essential feature for using Scuba in contexts where the container needs to forward ports from the host to the container (for instance, during web server development), or running in Docker's `privileged` mode to run applications that need KVM.

While the command-line argument is fantastic, the fact that there is no `.scuba.yml` counterpart means that instead of a developer just running `scuba run`, they have to do `scuba -d="-p 80:80 --privileged" run`. This also means that the additional arguments have to be documented and known by every developer, which increases friction for a developer on a new project. It would be much simpler if those arguments could be specified in the `.scuba.yml` file so that a new developer would be able to just run `scuba run` and have all of the additional arguments passed to Docker automatically.

The new `docker_args` key could either be specified at the top level, or inside of an alias:
```yaml
image: debian:buster
docker_args: --privileged

aliases:
  run_webserver:
    docker_args: -p 80:80 -p 443:443
    script:
        - python3 -m myKVMwebserver
```

Or, `docker_args` could take a list of arguments, more like:
```yaml
docker_args:
  - --privileged
  - -p 80:80
```
I dislike the list approach because it appears to be less readable to me.

I think this change would also (at least partially) address #140, #141, and #142.

Discussion:

Patch:
diff --git a/doc/yaml-reference.md b/doc/yaml-reference.md
index 8570305..082d5ef 100644
--- a/doc/yaml-reference.md
+++ b/doc/yaml-reference.md
@@ -40,6 +40,25 @@ environment:
   - SECRET
 ```
 
+### `docker_args`
+
+The optional `docker_args` node allows additional docker arguments to be
+specified.
+
+Example:
+```yaml
+docker_args: --privileged -v "/tmp/hello world:/tmp/hello world"
+```
+
+The value of `docker_args` is parsed as shell command line arguments using
+[`shlex.split`](https://docs.python.org/3/library/shlex.html#shlex.split).
+
+The previous example could be equivalently written in YAML's [single-quoted
+style](https://yaml.org/spec/1.2/spec.html#id2788097):
+
+```yaml
+docker_args: '--privileged -v "/tmp/hello world:/tmp/hello world"'
+```
 
 ### `aliases`
 
@@ -90,6 +109,47 @@ aliases:
       - echo $FOO $BAR
 ```
 
+Aliases can extend the top-level `docker_args`. The following example will
+produce the docker arguments `--privileged -v /tmp/bar:/tmp/bar` when executing
+the `example` alias:
+
+```yaml
+docker_args: --privileged
+aliases:
+  example:
+    docker_args: -v /tmp/bar:/tmp/bar
+    script:
+      - ls -l /tmp/
+```
+
+Aliases can also opt to override the top-level `docker_args`, replacing it with
+a new value. This is achieved with the `!override` tag:
+
+```yaml
+docker_args: -v /tmp/foo:/tmp/foo
+aliases:
+  example:
+    docker_args: !override -v /tmp/bar:/tmp/bar
+    script:
+      - ls -l /tmp/
+```
+
+The content of the `docker_args` key is re-parsed as YAML in order to allow
+combining the `!override` tag with other tags; however, this requires quoting
+the value, since YAML forbids a plain-style scalar from beginning with a `!`
+(see [the spec](https://yaml.org/spec/1.2/spec.html#id2788859)). In the next
+example, the top-level alias is replaced with an explicit `!!null` tag, so
+that no additional arguments are passed to docker when executing the `example`
+alias:
+
+```yaml
+docker_args: -v /tmp/foo:/tmp/foo
+aliases:
+  example:
+    docker_args: !override '!!null'
+    script:
+      - ls -l /tmp/
+```
 
 ### `hooks`
 
diff --git a/scuba/__main__.py b/scuba/__main__.py
index c14f63c..c7eb078 100644
--- a/scuba/__main__.py
+++ b/scuba/__main__.py
@@ -131,7 +131,8 @@ class ScubaDive:
         # These will be added to docker run cmdline
         self.env_vars = env
         self.volumes = []
-        self.options = docker_args or []
+        self.options = []
+        self.docker_args = docker_args or []
         self.workdir = None
 
         self.__locate_scubainit()
@@ -415,6 +416,12 @@ class ScubaDive:
 
         args += self.options
 
+        # Precedence: (1) command-line -d, (2) alias docker_args, (3) top-level docker_args
+        if self.docker_args:
+            args += self.docker_args
+        elif self.context.docker_args is not None:
+            args += self.context.docker_args
+
         # Docker image
         args.append(self.context.image)
 
diff --git a/scuba/config.py b/scuba/config.py
index 16f0a63..1c85e3a 100644
--- a/scuba/config.py
+++ b/scuba/config.py
@@ -1,3 +1,4 @@
+import collections
 import os
 import yaml
 import re
@@ -12,10 +13,35 @@ class ConfigError(Exception):
 class ConfigNotFoundError(ConfigError):
     pass
 
+class OverrideMixin:
+    '''
+    A mixin class that indicates an instance's value should override something
+
+    This class is mixed into objects loaded from YAML with an !override tag,
+    and any object can be checked if it is an OverrideMixin using isinstance().
+    '''
+    pass
+
+class OverrideNone(OverrideMixin):
+    '''
+    Represents a None value that also has Override behavior
+    '''
+    def __bool__(self):
+        return False
+
+class OverrideList(collections.UserList, OverrideMixin):
+    pass
+
+class OverrideStr(str, OverrideMixin):
+    pass
+
 # http://stackoverflow.com/a/9577670
 class Loader(yaml.SafeLoader):
-    def __init__(self, stream):
-        self._root = os.path.split(stream.name)[0]
+    def __init__(self, stream, root=None):
+        if root is None:
+            self._root = os.path.split(stream.name)[0]
+        else:
+            self._root = root
         self._cache = dict()
         super().__init__(stream)
 
@@ -65,7 +91,31 @@ class Loader(yaml.SafeLoader):
             raise yaml.YAMLError('Key "{}" not found in {}'.format(key, filename))
         return cur
 
+    def override(self, node):
+        '''
+        Implements !override constructor
+        '''
+        # Load the content from the node, as a scalar
+        content = self.construct_scalar(node)
+
+        # Dynamically add an OverrideMixin to the resulting object's type
+        obj = yaml.load(content, lambda s: Loader(s, root=self._root))
+        if obj is None:
+            obj = OverrideNone()
+        else:
+            objtype = type(obj)
+            mixin_type = type('Override' + objtype.__name__, (objtype, OverrideMixin), dict())
+
+            try:
+                obj.__class__ = mixin_type
+            except TypeError:
+                # Primitive classes (e.g., int, str) don't support __class__ assignment
+                obj = mixin_type(obj)
+
+        return obj
+
 Loader.add_constructor('!from_yaml', Loader.from_yaml)
+Loader.add_constructor('!override', Loader.override)
 
 
 def find_config():
@@ -150,24 +200,24 @@ def _process_environment(node, name):
 
     return result
 
-def _get_entrypoint(data):
+def _get_nullable_str(data, key):
     # N.B. We can't use data.get() here, because that might return
-    # None, leading to ambiguity between entrypoint being absent or set
+    # None, leading to ambiguity between the key being absent or set
     # to a null value.
     #
     # "Note that a null is different from an empty string and that a
     # mapping entry with some key and a null value is valid and
     # different from not having that key in the mapping."
     #   - http://yaml.org/type/null.html
-    key = 'entrypoint'
-
     if not key in data:
         return None
 
     ep = data[key]
 
     # We represent a null value as an empty string.
-    if ep is None:
+    if isinstance(ep, OverrideNone):
+        ep = OverrideStr('')
+    elif ep is None:
         ep = ''
 
     if not isinstance(ep, str):
@@ -175,9 +225,22 @@ def _get_entrypoint(data):
                 key, type(ep).__name__))
     return ep
 
+def _get_entrypoint(data):
+    return _get_nullable_str(data, 'entrypoint')
+
+def _get_docker_args(data):
+    args = _get_nullable_str(data, 'docker_args')
+
+    if args is not None:
+        override = isinstance(args, OverrideMixin)
+        args = shlex.split(args)
+        if override:
+            args = OverrideList(args)
+
+    return args
 
 class ScubaAlias:
-    def __init__(self, name, script, image, entrypoint, environment, shell, as_root):
+    def __init__(self, name, script, image, entrypoint, environment, shell, as_root, docker_args):
         self.name = name
         self.script = script
         self.image = image
@@ -185,6 +248,7 @@ class ScubaAlias:
         self.environment = environment
         self.shell = shell
         self.as_root = as_root
+        self.docker_args = docker_args
 
     @classmethod
     def from_dict(cls, name, node):
@@ -194,9 +258,11 @@ class ScubaAlias:
         environment = None
         shell = None
         as_root = False
+        docker_args = None
 
         if isinstance(node, dict):  # Rich alias
             image = node.get('image')
+            docker_args = _get_docker_args(node)
             entrypoint = _get_entrypoint(node)
             environment = _process_environment(
                     node.get('environment'),
@@ -204,14 +270,14 @@ class ScubaAlias:
             shell = node.get('shell')
             as_root = node.get('root', as_root)
 
-        return cls(name, script, image, entrypoint, environment, shell, as_root)
+        return cls(name, script, image, entrypoint, environment, shell, as_root, docker_args)
 
 class ScubaContext:
     pass
 
 class ScubaConfig:
     def __init__(self, **data):
-        optional_nodes = ('image','aliases','hooks','entrypoint','environment','shell')
+        optional_nodes = ('image','aliases','hooks','entrypoint','environment','shell','docker_args')
 
         # Check for unrecognized nodes
         extra = [n for n in data if not n in optional_nodes]
@@ -222,6 +288,7 @@ class ScubaConfig:
         self._image = data.get('image')
         self._shell = data.get('shell', DEFAULT_SHELL)
         self._entrypoint = _get_entrypoint(data)
+        self._docker_args = _get_docker_args(data)
         self._load_aliases(data)
         self._load_hooks(data)
         self._environment = self._load_environment(data)
@@ -277,6 +344,10 @@ class ScubaConfig:
     def shell(self):
         return self._shell
 
+    @property
+    def docker_args(self):
+        return self._docker_args
+
 
     def process_command(self, command, image=None, shell=None):
         '''Processes a user command using aliases
@@ -297,6 +368,7 @@ class ScubaConfig:
         result.environment = self.environment.copy()
         result.shell = self.shell
         result.as_root = False
+        result.docker_args = self.docker_args
 
         if command:
             alias = self.aliases.get(command[0])
@@ -315,6 +387,11 @@ class ScubaConfig:
                 if alias.as_root:
                     result.as_root = True
 
+                if isinstance(alias.docker_args, OverrideMixin) or result.docker_args is None:
+                    result.docker_args = alias.docker_args
+                elif alias.docker_args is not None:
+                    result.docker_args.extend(alias.docker_args)
+
                 # Merge/override the environment
                 if alias.environment:
                     result.environment.update(alias.environment)


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index c94960b..fb24e28 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -488,6 +488,73 @@ class TestConfig:
         result = cfg.process_command([], image=override_image_name)
         assert result.image == override_image_name
 
+    def test_process_command_alias_extends_docker_args(self):
+        '''aliases can extend the docker_args'''
+        cfg = scuba.config.ScubaConfig(
+                image = 'default',
+                docker_args = '--privileged',
+                aliases = dict(
+                    apple = dict(
+                        script = [
+                            'banana cherry "pie is good"',
+                        ],
+                        docker_args = '-v /tmp/:/tmp/',
+                    ),
+                ),
+            )
+        result = cfg.process_command(['apple'])
+        assert result.docker_args == ['--privileged', '-v', '/tmp/:/tmp/']
+
+    def test_process_command_alias_overrides_docker_args(self):
+        '''aliases can override the docker_args'''
+        cfg = scuba.config.ScubaConfig(
+                image = 'default',
+                docker_args = '--privileged',
+                aliases = dict(
+                    apple = dict(
+                        script = [
+                            'banana cherry "pie is good"',
+                        ],
+                        docker_args = scuba.config.OverrideStr('-v /tmp/:/tmp/'),
+                    ),
+                ),
+            )
+        result = cfg.process_command(['apple'])
+        assert result.docker_args == ['-v', '/tmp/:/tmp/']
+
+    def test_process_command_alias_overrides_docker_args_with_empty(self):
+        '''aliases can override the docker_args'''
+        cfg = scuba.config.ScubaConfig(
+                image = 'default',
+                docker_args = '--privileged',
+                aliases = dict(
+                    apple = dict(
+                        script = [
+                            'banana cherry "pie is good"',
+                        ],
+                        docker_args = scuba.config.OverrideStr(''),
+                    ),
+                ),
+            )
+        result = cfg.process_command(['apple'])
+        assert result.docker_args == []
+
+    def test_process_command_alias_inherits_top_docker_args(self):
+        '''aliases inherit the top-level docker_args if not specified'''
+        cfg = scuba.config.ScubaConfig(
+                image = 'default',
+                docker_args = '--privileged',
+                aliases = dict(
+                    apple = dict(
+                        script = [
+                            'banana cherry "pie is good"',
+                        ],
+                    ),
+                ),
+            )
+        result = cfg.process_command(['apple'])
+        assert result.docker_args == ['--privileged']
+
     ############################################################################
     # Hooks
 
@@ -758,3 +825,192 @@ class TestConfig:
 
         config = scuba.config.load_config('.scuba.yml')
         assert config.aliases['testalias'].entrypoint == 'use_this_ep'
+
+    ############################################################################
+    # docker_args
+
+    def test_docker_args_not_set(self):
+        '''docker_args can be missing'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.docker_args is None
+
+    def test_docker_args_invalid(self):
+        '''docker_args of incorrect type raises ConfigError'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: 666
+                ''')
+
+        self._invalid_config('must be a string')
+
+    def test_docker_args_null(self):
+        '''docker_args can be set to null'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args:
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.docker_args == []
+
+    def test_docker_args_set_empty_string(self):
+        '''docker_args can be set to empty string'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: ''
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.docker_args == []  # '' -> [] after shlex.split()
+
+    def test_docker_args_set(self):
+        '''docker_args can be set'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.docker_args == ['--privileged']
+
+    def test_docker_args_set_multi(self):
+        '''docker_args can be set to multiple args'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged -v /tmp/:/tmp/
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.docker_args == ['--privileged', '-v', '/tmp/:/tmp/']
+
+    def test_alias_docker_args_null(self):
+        '''docker_args can be set to null via alias'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged
+                aliases:
+                  testalias:
+                    docker_args:
+                    script:
+                      - ugh
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.aliases['testalias'].docker_args == []
+
+    def test_alias_docker_args_empty_string(self):
+        '''docker_args can be set to empty string via alias'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged
+                aliases:
+                  testalias:
+                    docker_args: ''
+                    script:
+                      - ugh
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.aliases['testalias'].docker_args == []
+
+    def test_alias_docker_args_set(self):
+        '''docker_args can be set via alias'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged
+                aliases:
+                  testalias:
+                    docker_args: -v /tmp/:/tmp/
+                    script:
+                      - ugh
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.aliases['testalias'].docker_args == ['-v', '/tmp/:/tmp/']
+
+    def test_alias_docker_args_override(self):
+        '''docker_args can be tagged for override'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged
+                aliases:
+                  testalias:
+                    docker_args: !override -v /tmp/:/tmp/
+                    script:
+                      - ugh
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.aliases['testalias'].docker_args == ['-v', '/tmp/:/tmp/']
+        assert isinstance(config.aliases['testalias'].docker_args, scuba.config.OverrideMixin)
+
+    def test_alias_docker_args_override_implicit_null(self):
+        '''docker_args can be overridden with an implicit null value'''
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged
+                aliases:
+                  testalias:
+                    docker_args: !override
+                    script:
+                      - ugh
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.aliases['testalias'].docker_args == []
+        assert isinstance(config.aliases['testalias'].docker_args, scuba.config.OverrideMixin)
+
+    def test_alias_docker_args_override_from_yaml(self):
+        '''!override tag can be applied before a !from_yaml tag'''
+        with open('args.yml', 'w') as f:
+            f.write('args: -v /tmp/:/tmp/\n')
+
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged
+                aliases:
+                  testalias:
+                    docker_args: !override '!from_yaml args.yml args'
+                    script:
+                      - ugh
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.aliases['testalias'].docker_args == ['-v', '/tmp/:/tmp/']
+        assert isinstance(config.aliases['testalias'].docker_args, scuba.config.OverrideMixin)
+
+    def test_alias_docker_args_from_yaml_override(self):
+        '''!override tag can be applied inside of a !from_yaml tag'''
+        with open('args.yml', 'w') as f:
+            f.write('args: !override -v /tmp/:/tmp/\n')
+
+        with open('.scuba.yml', 'w') as f:
+            f.write(r'''
+                image: na
+                docker_args: --privileged
+                aliases:
+                  testalias:
+                    docker_args: !from_yaml args.yml args
+                    script:
+                      - ugh
+                ''')
+
+        config = scuba.config.load_config('.scuba.yml')
+        assert config.aliases['testalias'].docker_args == ['-v', '/tmp/:/tmp/']
+        assert isinstance(config.aliases['testalias'].docker_args, scuba.config.OverrideMixin)
diff --git a/tests/test_main.py b/tests/test_main.py
index 58e93bf..7e053d0 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -414,6 +414,29 @@ class TestMain:
 
         assert_str_equalish(out, data)
 
+    def test_arbitrary_docker_args_override_config(self):
+        '''Verify -d overrides any docker arguments in the config'''
+
+        with open('.scuba.yml', 'w') as f:
+            f.write('image: {}\n'.format(DOCKER_IMAGE))
+
+            # If this arg takes effect, the output of ls /lorem/ is no longer just "ipsum"
+            f.write('docker_args: {}\n'.format('-v="/lorem/default:/lorem/default"'))
+
+        data = 'Lorem ipsum dolor sit amet'
+        data_path = '/lorem/ipsum'
+
+        with NamedTemporaryFile(mode='wt') as tempf:
+            tempf.write(data)
+            tempf.flush()
+
+            args = [
+                '-d=-v {}:{}:ro,z'.format(tempf.name, data_path),
+                'ls', '/lorem',
+            ]
+            out, _ = self.run_scuba(args)
+
+        assert_str_equalish(out, 'ipsum')
 
     def test_nested_sript(self):
         '''Verify nested scripts works'''