launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/tempita-power-control into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/tempita-power-control/+merge/109391
-- 
https://code.launchpad.net/~allenap/maas/tempita-power-control/+merge/109391
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/tempita-power-control into lp:maas.

=== modified file 'src/provisioningserver/power/poweraction.py'
--- src/provisioningserver/power/poweraction.py	2012-05-31 10:56:10 +0000
+++ src/provisioningserver/power/poweraction.py	2012-06-08 16:08:20 +0000
@@ -21,6 +21,7 @@
 import subprocess
 
 from celeryconfig import POWER_TEMPLATES_DIR
+from provisioningserver.utils import ShellTemplate
 
 
 class UnknownPowerType(Exception):
@@ -50,17 +51,14 @@
         self.power_type = power_type
 
     def get_template(self):
-        with open(self.path, "r") as f:
-            template = f.read()
-        return template
+        with open(self.path, "rb") as f:
+            return ShellTemplate(f.read(), name=self.path)
 
     def render_template(self, template, **kwargs):
         try:
-            rendered = template % kwargs
-        except KeyError, e:
-            raise PowerActionFail(
-                "Template is missing at least the %s parameter." % e.message)
-        return rendered
+            return template.substitute(kwargs)
+        except NameError as error:
+            raise PowerActionFail(*error.args)
 
     def run_shell(self, commands):
         """Execute raw shell script (as rendered from a template).

=== modified file 'src/provisioningserver/power/templates/ether_wake.template'
--- src/provisioningserver/power/templates/ether_wake.template	2012-05-29 08:07:16 +0000
+++ src/provisioningserver/power/templates/ether_wake.template	2012-06-08 16:08:20 +0000
@@ -1,5 +1,10 @@
-mac='%(mac)s'
-power_change='%(power_change)s'
+# -*- mode: shell-script -*-
+#
+# Control node power through WOL, via `wakeonlan` or `etherwake`.
+#
+
+mac={{mac}}
+power_change={{power_change}}
 
 if [ "${power_change}" != 'on' ]
 then

=== modified file 'src/provisioningserver/power/templates/virsh.template'
--- src/provisioningserver/power/templates/virsh.template	2012-05-31 07:19:38 +0000
+++ src/provisioningserver/power/templates/virsh.template	2012-06-08 16:08:20 +0000
@@ -1,10 +1,13 @@
+# -*- mode: shell-script -*-
+#
 # Control virtual system's "power" through virsh.
+#
 
 # Parameters.
-power_change="%(power_change)s"
-virsh_url="%(virsh_url)s"
-system_id="%(system_id)s"
-virsh="%(virsh)s"
+power_change={{power_change}}
+virsh_url={{virsh_url}}
+system_id={{system_id}}
+virsh={{virsh}}
 
 
 # Choose command for virsh to make the requested power change happen.

=== modified file 'src/provisioningserver/power/tests/test_poweraction.py'
--- src/provisioningserver/power/tests/test_poweraction.py	2012-05-31 07:19:38 +0000
+++ src/provisioningserver/power/tests/test_poweraction.py	2012-06-08 16:08:20 +0000
@@ -14,7 +14,7 @@
 __all__ = []
 
 import os
-from textwrap import dedent
+import re
 
 from celeryconfig import POWER_TEMPLATES_DIR
 from maastesting.factory import factory
@@ -25,7 +25,11 @@
     PowerActionFail,
     UnknownPowerType,
     )
-from testtools.matchers import FileContains
+from provisioningserver.utils import ShellTemplate
+from testtools.matchers import (
+    FileContains,
+    MatchesRegex,
+    )
 
 
 class TestPowerAction(TestCase):
@@ -51,29 +55,32 @@
     def test_get_template(self):
         # get_template() should find and read the template file.
         pa = PowerAction(POWER_TYPE.WAKE_ON_LAN)
+        template = pa.get_template()
+        self.assertIsInstance(template, ShellTemplate)
         with open(pa.path, "r") as f:
-            template = f.read()
-        self.assertEqual(template, pa.get_template())
+            template_content = f.read()
+        self.assertEqual(template_content, template.content)
 
     def test_render_template(self):
         # render_template() should take a template string and substitue
         # its variables.
         pa = PowerAction(POWER_TYPE.WAKE_ON_LAN)
-        template = "template: %(mac)s"
+        template = ShellTemplate("template: {{mac}}")
         rendered = pa.render_template(template, mac="mymac")
-        self.assertEqual(
-            template % dict(mac="mymac"), rendered)
+        self.assertEqual("template: mymac", rendered)
 
     def test_render_template_raises_PowerActionFail(self):
         # If not enough arguments are supplied to fill in template
         # variables then a PowerActionFail is raised.
         pa = PowerAction(POWER_TYPE.WAKE_ON_LAN)
-        template = "template: %(mac)s"
+        template = ShellTemplate(
+            "template: {{mac}}", name=factory.getRandomString())
         exception = self.assertRaises(
             PowerActionFail, pa.render_template, template)
-        self.assertEqual(
-            "Template is missing at least the mac parameter.",
-            exception.message)
+        self.assertThat(
+            exception.message, MatchesRegex(
+                "name 'mac' is not defined at line \d+ column \d+ "
+                "in file %s" % re.escape(template.name)))
 
     def _create_template_file(self, template):
         return self.make_file("testscript.sh", template)
@@ -89,13 +96,10 @@
         # Create a template in a temp dir.
         tempdir = self.make_dir()
         output_file = os.path.join(tempdir, "output")
-        template = dedent("""\
-            #!/bin/sh
-            echo working %(mac)s >""")
-        template += output_file
+        template = "echo working {{mac}} > {{outfile}}"
         path = self._create_template_file(template)
 
-        self.run_action(path, mac="test")
+        self.run_action(path, mac="test", outfile=output_file)
         self.assertThat(output_file, FileContains("working test\n"))
 
     def test_execute_raises_PowerActionFail_when_script_fails(self):

=== added file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-06-08 16:08:20 +0000
@@ -0,0 +1,45 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test `provisioningserver.utils`."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+from provisioningserver.utils import (
+    Safe,
+    ShellTemplate,
+    )
+
+
+class TestSafe(TestCase):
+    """Test `Safe`."""
+
+    def test_value(self):
+        something = object()
+        safe = Safe(something)
+        self.assertIs(something, safe.value)
+
+    def test_repr(self):
+        string = factory.getRandomString()
+        safe = Safe(string)
+        self.assertEqual("<Safe %r>" % string, repr(safe))
+
+
+class TestShellTemplate(TestCase):
+    """Test `ShellTemplate`."""
+
+    def test_substitute(self):
+        # Substitutions will be shell-escaped, unless marked `safe`.
+        template = ShellTemplate("{{a}} {{b|safe}} {{safe(c)}}")
+        expected = "'1 2 3' a b c $ ! ()"
+        observed = template.substitute(a="1 2 3", b="a b c", c="$ ! ()")
+        self.assertEqual(expected, observed)

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-04-16 10:00:51 +0000
+++ src/provisioningserver/utils.py	2012-06-08 16:08:20 +0000
@@ -12,11 +12,14 @@
 __metaclass__ = type
 __all__ = [
     "deferred",
+    "ShellTemplate",
     "xmlrpc_export",
     ]
 
 from functools import wraps
+from pipes import quote
 
+import tempita
 from twisted.internet.defer import maybeDeferred
 from zope.interface.interface import Method
 
@@ -53,3 +56,41 @@
                 setattr(cls, "xmlrpc_%s" % name, method)
         return cls
     return decorate
+
+
+class Safe:
+    """An object that should not be quoted when rendered."""
+
+    def __init__(self, value):
+        self.value = value
+
+    def __repr__(self):
+        return "<%s %r>" % (
+            self.__class__.__name__, self.value)
+
+
+
+class ShellTemplate(tempita.Template):
+    """A Tempita template specialised for writing shell scripts.
+
+    By default, substitutions will be escaped using `pipes.quote`, unless
+    they're marked as safe. This can be done using Tempita's filter syntax::
+
+      {{foobar|safe}}
+
+    or as a plain Python expression::
+
+      {{safe(foobar)}}
+
+    """
+
+    default_namespace = dict(
+        tempita.Template.default_namespace,
+        safe=Safe)
+
+    def _repr(self, value, pos):
+        """Shell-quote the value by default."""
+        is_safe = isinstance(value, Safe)
+        value = super(ShellTemplate, self)._repr(
+            value.value if is_safe else value, pos)
+        return value if is_safe else quote(value)