Repository: JonathonReinhart/scuba (python)

Issue:
Allow `script` to be a single string value
Currently, this is invalid:
```yaml
aliases:
  foo:
    image: xyz
    script: echo Hello
```

Pull request:
Allow 'script' to be a single string in the Common Script Schema
Fixes #102 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7943d52..346ce8c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,10 @@ All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
+### Changed
+- Allow `script` to be a single string value in the "common script schema"
+  which applies to hooks and aliases (#102)
+
 ### Fixed
 - Display nicer error message if no command is given and image doesn't specify a `Cmd` (#104)
 - Don't mangle && in scripts (#100)
diff --git a/doc/yaml-reference.md b/doc/yaml-reference.md
index 6adece5..c8021ed 100644
--- a/doc/yaml-reference.md
+++ b/doc/yaml-reference.md
@@ -54,7 +54,7 @@ hooks:
 
 ## Common script schema
 Several parts of `.scuba.yml` which define "scripts" use a common schema.
-The *common script schema* can define a "script" in one of two forms:
+The *common script schema* can define a "script" in one of several forms:
 
 The *simple* form is simply a single string value:
 ```yaml
@@ -63,7 +63,14 @@ hooks:
 ```
 
 The *complex* form is a mapping, which must contain a `script` subkey, whose
-value is a list of strings making up the script:
+value is either single string value
+```yaml
+hooks:
+  root:
+    script: echo hello
+```
+
+... or a list of strings making up the script:
 ```yaml
 hooks:
   root:
diff --git a/example/per_alias_image/.scuba.yml b/example/per_alias_image/.scuba.yml
index 9b9bae0..df73881 100644
--- a/example/per_alias_image/.scuba.yml
+++ b/example/per_alias_image/.scuba.yml
@@ -2,11 +2,12 @@ image: !from_yaml ../common.yml image
 aliases:
 
   # This one inherits the default, top-level 'image'
+  # and specifies "script" as a string
   default:
-    script:
-      - cat /etc/os-release
+    script: cat /etc/os-release
 
   # This one specifies a different image to use
+  # and specifies "script" as a list
   different:
     image: alpine
     script:
diff --git a/scuba/config.py b/scuba/config.py
index d1d33eb..b8e0417 100644
--- a/scuba/config.py
+++ b/scuba/config.py
@@ -118,10 +118,13 @@ def _process_script_node(node, name):
         if not script:
             raise ConfigError("{0}: must have a 'script' subkey".format(name))
 
-        if not isinstance(script, list):
-            raise ConfigError("{0}.script: must be a list".format(name))
+        if isinstance(script, list):
+            return script
 
-        return script
+        if isinstance(script, basestring):
+            return [script]
+
+        raise ConfigError("{0}.script: must be a string or list".format(name))
 
     raise ConfigError("{0}: must be string or dict".format(name))
 


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 8e4d7c9..118379e 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -8,26 +8,52 @@ from unittest import TestCase
 import logging
 import os
 from os.path import join
-from tempfile import mkdtemp
 from shutil import rmtree
 
 from scuba.utils import shlex_split
 import scuba.config
 
-class TestConfig(TestCase):
-    def setUp(self):
-        self.orig_path = os.getcwd()
 
-        self.path = mkdtemp('scubatest')
-        os.chdir(self.path)
-        logging.info('Temp path: ' + self.path)
-
-    def tearDown(self):
-        rmtree(self.path)
-        self.path = None
-
-        os.chdir(self.orig_path)
-        self.orig_path = None
+class TestCommonScriptSchema(TmpDirTestCase):
+    def test_simple(self):
+        '''Simple form: value is a string'''
+        node = 'foo'
+        result = scuba.config._process_script_node(node, 'dontcare')
+        assert_equals(result, ['foo'])
+
+    def test_script_key_string(self):
+        '''Value is a mapping: script is a string'''
+        node = dict(
+            script = 'foo',
+            otherkey = 'other',
+        )
+        result = scuba.config._process_script_node(node, 'dontcare')
+        assert_equals(result, ['foo'])
+
+    def test_script_key_list(self):
+        '''Value is a mapping: script is a list'''
+        node = dict(
+            script = [
+                'foo',
+                'bar',
+            ],
+            otherkey = 'other',
+        )
+        result = scuba.config._process_script_node(node, 'dontcare')
+        assert_equals(result, ['foo', 'bar'])
+
+    def test_script_key_mapping_invalid(self):
+        '''Value is a mapping: script is a mapping (invalid)'''
+        node = dict(
+            script = dict(
+                whatisthis = 'idontknow',
+            ),
+        )
+        assert_raises(scuba.config.ConfigError,
+                scuba.config._process_script_node, node, 'dontcare')
+
+
+class TestConfig(TmpDirTestCase):
 
     ######################################################################
     # Find config
@@ -335,19 +361,6 @@ class TestConfig(TestCase):
 
         assert_raises(scuba.config.ConfigError, scuba.config.load_config, '.scuba.yml')
 
-    # TODO: Any reason this shouldn't be valid?
-    def test_hooks_invalid_script_type(self):
-        '''hooks with string "script" are invalid'''
-        with open('.scuba.yml', 'w') as f:
-            f.write('''
-                image: na
-                hooks:
-                  user:
-                    script: this should be in a list under script
-                ''')
-
-        assert_raises(scuba.config.ConfigError, scuba.config.load_config, '.scuba.yml')
-
     def test_hooks_missing_script(self):
         '''hooks with dict, but missing "script" are invalid'''
         with open('.scuba.yml', 'w') as f:
diff --git a/tests/test_main.py b/tests/test_main.py
index 00f8d4b..588d0e9 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -11,8 +11,7 @@ except ImportError:
 import logging
 import os
 import sys
-from tempfile import mkdtemp, TemporaryFile, NamedTemporaryFile
-from shutil import rmtree
+from tempfile import TemporaryFile, NamedTemporaryFile
 import subprocess
 
 import scuba.__main__ as main
@@ -22,22 +21,7 @@ import scuba
 
 DOCKER_IMAGE = 'debian:8.2'
 
-class TestMain(TestCase):
-    def setUp(self):
-        # Run each test in its own temp directory
-        self.orig_path = os.getcwd()
-        self.path = mkdtemp('scubatest')
-        os.chdir(self.path)
-        logging.info('Temp path: ' + self.path)
-
-
-    def tearDown(self):
-        # Restore the working dir and cleanup the temp one
-        rmtree(self.path)
-        self.path = None
-        os.chdir(self.orig_path)
-        self.orig_path = None
-
+class TestMain(TmpDirTestCase):
 
     def run_scuba(self, args, exp_retval=0, mock_isatty=False, stdin=None):
         '''Run scuba, checking its return value
diff --git a/tests/utils.py b/tests/utils.py
index 1dfaa74..1444b3d 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -4,6 +4,8 @@ from nose.tools import *
 from os.path import normpath
 import tempfile
 import shutil
+import unittest
+import logging
 
 def assert_set_equal(a, b):
     assert_equal(set(a), set(b))
@@ -100,3 +102,20 @@ class RedirStd(object):
 
         if self.orig_stderr:
             sys.stderr = self.orig_stderr
+
+
+class TmpDirTestCase(unittest.TestCase):
+    def setUp(self):
+        # Run each test in its own temp directory
+        self.orig_path = os.getcwd()
+        self.path = tempfile.mkdtemp('scubatest')
+        os.chdir(self.path)
+        logging.info('Temp path: ' + self.path)
+
+
+    def tearDown(self):
+        # Restore the working dir and cleanup the temp one
+        shutil.rmtree(self.path)
+        self.path = None
+        os.chdir(self.orig_path)
+        self.orig_path = None