Add some more sysconfig tests + pylint cleanups.

3 files changed, 59 insertions, 24 deletions

diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index bafa69d3..fa7cc1ca 100644
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -132,7 +132,8 @@ class Distro(object):
         # temporarily (until reboot so it should
         # not be depended on). Use the write
         # hostname functions for 'permanent' adjustments.
-        LOG.debug("Non-persistently setting the system hostname to %s", hostname)
+        LOG.debug("Non-persistently setting the system hostname to %s",
+                  hostname)
         try:
             util.subp(['hostname', hostname])
         except util.ProcessExecutionError:
diff --git a/cloudinit/distros/parsers/sys_conf.py b/cloudinit/distros/parsers/sys_conf.py
index 1cefb8bc..5cd765fc 100644
--- a/cloudinit/distros/parsers/sys_conf.py
+++ b/cloudinit/distros/parsers/sys_conf.py
@@ -22,7 +22,7 @@ import pipes
 import re
 
 # This library is used to parse/write
-# out the various sysconfig files edited
+# out the various sysconfig files edited (best attempt effort)
 #
 # It has to be slightly modified though
 # to ensure that all values are quoted/unquoted correctly
@@ -30,11 +30,26 @@ import re
 # bash scripts...
 import configobj
 
+# See: http://pubs.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap08.html
+# or look at the 'param_expand()' function in the subst.c file in the bash
+# source tarball...
+SHELL_VAR_RULE = r'[a-zA-Z_]+[a-zA-Z0-9_]*'
+SHELL_VAR_REGEXES = [
+    # Basic variables
+    re.compile(r"\$" + SHELL_VAR_RULE),
+    # Things like $?, $0, $-, $@
+    re.compile(r"\$[0-9#\?\-@\*]"),
+    # Things like ${blah:1} - but this one
+    # gets very complex so just try the 
+    # simple path
+    re.compile(r"\$\{.+\}"),
+]
+
 
 def _contains_shell_variable(text):
-    if (re.search(r"\$\{.+\}", text) or
-        re.search(r"\$[a-zA-Z_]+[a-zA-Z0-9_]*", text)):
-           return True
+    for r in SHELL_VAR_REGEXES:
+        if r.search(text):
+            return True
     return False
 
 
@@ -58,33 +73,36 @@ class SysConf(configobj.ConfigObj):
             raise ValueError('Value "%s" is not a string' % (value))
         if len(value) == 0:
             return ''
-        quot_func = (lambda x: str(x))
+        quot_func = None
         if value[0] in ['"', "'"] and value[-1] in ['"', "'"]:
             if len(value) == 1:
-                quot_func = (lambda x: self._get_single_quote(x) % x)
+                quot_func = (lambda x:
+                                self._get_single_quote(x) % x)
         else:
             # Quote whitespace if it isn't the start + end of a shell command
-            white_space_ok = False
             if value.strip().startswith("$(") and value.strip().endswith(")"):
-                white_space_ok = True
-            if re.search(r"[\t\r\n ]", value) and not white_space_ok:
-                if _contains_shell_variable(value):
-                    # If it contains shell variables then we likely want to 
-                    # leave it alone since the pipes.quote function likes to
-                    # use single quotes which won't get expanded...
-                    if re.search(r"[\n\"']", value):
-                        quot_func = (lambda x: self._get_triple_quote(x) % x)
+                pass
+            else:
+                if re.search(r"[\t\r\n ]", value):
+                    if _contains_shell_variable(value):
+                        # If it contains shell variables then we likely want to
+                        # leave it alone since the pipes.quote function likes
+                        # to use single quotes which won't get expanded...
+                        if re.search(r"[\n\"']", value):
+                            quot_func = (lambda x:
+                                            self._get_triple_quote(x) % x)
+                        else:
+                            quot_func = (lambda x:
+                                            self._get_single_quote(x) % x)
                     else:
-                        quot_func = (lambda x: self._get_single_quote(x) % x)
-                else:
-                    quot_func = pipes.quote
+                        quot_func = pipes.quote
+        if not quot_func:
+            return value
         return quot_func(value)
 
     def _write_line(self, indent_string, entry, this_entry, comment):
         # Ensure it is formatted fine for
         # how these sysconfig scripts are used
-        if this_entry.startswith("'") or this_entry.startswith('"'):
-            val = this_entry
         val = self._decode_element(self._quote(this_entry))
         key = self._decode_element(self._quote(entry))
         cmnt = self._decode_element(comment)
@@ -93,4 +111,3 @@ class SysConf(configobj.ConfigObj):
                                self._a_to_u('='),
                                val,
                                cmnt)
-
diff --git a/tests/unittests/test_distros/test_sysconfig.py b/tests/unittests/test_distros/test_sysconfig.py
index 21e161ad..1e34909d 100644
--- a/tests/unittests/test_distros/test_sysconfig.py
+++ b/tests/unittests/test_distros/test_sysconfig.py
@@ -9,7 +9,8 @@ from cloudinit.distros.parsers.sys_conf import SysConf
 # http://content.hccfl.edu/pollock/AUnix1/SysconfigFilesDesc.txt
 
 class TestSysConfHelper(MockerTestCase):
-    def assertRegexpMatches(self, text, regexp):
+    # This function was added in 2.7, make it work for 2.6
+    def assertRegMatches(self, text, regexp):
         regexp = re.compile(regexp)
         self.assertTrue(regexp.search(text),
                         msg="%s must match %s!" % (text, regexp.pattern))
@@ -34,6 +35,21 @@ USEMD5=no'''
                                                  '-G ${DEVICE} rx 256 tx 256'))
         self.assertEquals(contents, str(conf))
 
+    def test_parse_shell_vars(self):
+        contents = 'USESMBAUTH=$XYZ'
+        conf = SysConf(contents.splitlines())
+        self.assertEquals(contents, str(conf))
+        conf = SysConf('')
+        conf['B'] = '${ZZ}d apples'
+        # Should be quoted
+        self.assertEquals('B="${ZZ}d apples"', str(conf))
+        conf = SysConf('')
+        conf['B'] = '$? d apples'
+        self.assertEquals('B="$? d apples"', str(conf))
+        contents = 'IPMI_WATCHDOG_OPTIONS="timeout=60"'
+        conf = SysConf(contents.splitlines())
+        self.assertEquals('IPMI_WATCHDOG_OPTIONS=timeout=60', str(conf))
+
     def test_parse_adjust(self):
         contents = 'IPV6TO4_ROUTING="eth0-:0004::1/64 eth1-:0005::1/64"'
         conf = SysConf(contents.splitlines())
@@ -43,7 +59,8 @@ USEMD5=no'''
         conf['IPV6TO4_ROUTING'] = "blah \tblah"
         contents2 = str(conf).strip()
         # Should be requoted due to whitespace
-        self.assertRegexpMatches(contents2, r'IPV6TO4_ROUTING=[\']blah\s+blah[\']')
+        self.assertRegMatches(contents2,
+                              r'IPV6TO4_ROUTING=[\']blah\s+blah[\']')
 
     def test_parse_no_adjust_shell(self):
         conf = SysConf(''.splitlines())