launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09311
[Merge] lp:~jtv/maas/pxe-templates-path into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/pxe-templates-path into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/pxe-templates-path/+merge/112505
Andres reported this problem when running the packaged version of MAAS: http://pastebin.ubuntu.com/1062430/
The reason is that celeryconfig.py configures some default template directories relative to its own location. That only works when running from the source tree, not when running from an installed instance — celeryconfig lives in “etc/.”
As discussed with roaksoax and rvba, this branch eliminates that problem. I did exactly the same thing for the PXE templates and the power templates, which have a lot of patterns in common: the code that needs these template paths hard-wires a default of “os.path.join(os.path.dirname(__file__), "templates").” That's the same path that celeryconfig specified, except celeryconfig doesn't have a good relative path back to the templates directory. It's much easier when navigate from the code that sits right next to the templates directory. Andres agreed to colocate the templates directory with the code that uses it, so that this simple path will work both in installed code and in a source tree.
With this change, the celery config can still _override_ the default path, but the default is None which makes the code fall back to the built-in default. Basically nothing changes. However I also made some of the tests force the celeryconfig setting to None, so that the tests will still pass on systems that have custom celeryconfig settings.
The tests also become slightly easier, if it does add a few, because path generation can now be tested in isolation and for the rest of the tests all that matters is that the templating code obeys the generated paths. I'm not entirely happy about using a property for figuring out the right path, but I thought I'd try to leave that as it was. I just followed and reinforced the already established pattern there.
I considered unifying the default-path code, but I'm not sure the time is quite right for that. It's actually very little code and who knows, we may still find reasons to do things differently between the PXE and power-action modules. Don't want to force those into a uniform straitjacket before we know it makes sense.
Jeroen
--
https://code.launchpad.net/~jtv/maas/pxe-templates-path/+merge/112505
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/pxe-templates-path into lp:maas.
=== modified file 'etc/celeryconfig.py'
--- etc/celeryconfig.py 2012-06-27 11:09:50 +0000
+++ etc/celeryconfig.py 2012-06-28 07:46:21 +0000
@@ -15,19 +15,15 @@
__metaclass__ = type
-import os
-
from maas import import_settings
-# Location of power action templates. Use an absolute path.
-POWER_TEMPLATES_DIR = os.path.join(
- os.path.dirname(__file__), os.pardir, "src", "provisioningserver", "power",
- "templates")
+# Location of power action templates. Use an absolute path, or leave as
+# None to use the templates installed with the running version of MAAS.
+POWER_TEMPLATES_DIR = None
-# Location of PXE config templates. Use an absolute path.
-PXE_TEMPLATES_DIR = os.path.join(
- os.path.dirname(__file__), os.pardir, "src", "provisioningserver", "pxe",
- "templates")
+# Location of PXE config templates. Use an absolute path, or leave as
+# None to use the templates installed with the running version of MAAS.
+PXE_TEMPLATES_DIR = None
# TFTP server's root directory.
TFTPROOT = "/var/lib/tftpboot"
=== modified file 'src/provisioningserver/power/poweraction.py'
--- src/provisioningserver/power/poweraction.py 2012-06-08 16:05:50 +0000
+++ src/provisioningserver/power/poweraction.py 2012-06-28 07:46:21 +0000
@@ -43,13 +43,24 @@
"""
def __init__(self, power_type):
- basedir = POWER_TEMPLATES_DIR
- self.path = os.path.join(basedir, power_type + ".template")
+ self.path = os.path.join(
+ self.template_basedir, power_type + ".template")
if not os.path.exists(self.path):
raise UnknownPowerType(power_type)
self.power_type = power_type
+ @property
+ def template_basedir(self):
+ """Directory where power templates are stored."""
+ if POWER_TEMPLATES_DIR is None:
+ # The power templates are installed into the same location
+ # as this file, and also live in the same directory as this
+ # file in the source tree.
+ return os.path.join(os.path.dirname(__file__), 'templates')
+ else:
+ return POWER_TEMPLATES_DIR
+
def get_template(self):
with open(self.path, "rb") as f:
return ShellTemplate(f.read(), name=self.path)
=== modified file 'src/provisioningserver/power/tests/test_poweraction.py'
--- src/provisioningserver/power/tests/test_poweraction.py 2012-06-25 09:03:14 +0000
+++ src/provisioningserver/power/tests/test_poweraction.py 2012-06-28 07:46:21 +0000
@@ -16,10 +16,10 @@
import os
import re
-from celeryconfig import POWER_TEMPLATES_DIR
from maastesting.factory import factory
from maastesting.testcase import TestCase
from provisioningserver.enum import POWER_TYPE
+import provisioningserver.power.poweraction
from provisioningserver.power.poweraction import (
PowerAction,
PowerActionFail,
@@ -35,6 +35,11 @@
class TestPowerAction(TestCase):
"""Tests for PowerAction."""
+ def configure_templates_dir(self, path=None):
+ """Configure POWER_TEMPLATES_DIR to `path`."""
+ self.patch(
+ provisioningserver.power.poweraction, 'POWER_TEMPLATES_DIR', path)
+
def test_init_raises_for_unknown_powertype(self):
powertype = factory.make_name("powertype", sep='')
self.assertRaises(
@@ -46,20 +51,45 @@
self.assertEqual(POWER_TYPE.WAKE_ON_LAN, pa.power_type)
def test_init_stores_template_path(self):
+ self.configure_templates_dir()
power_type = POWER_TYPE.WAKE_ON_LAN
- basedir = POWER_TEMPLATES_DIR
- path = os.path.join(basedir, power_type + ".template")
pa = PowerAction(power_type)
+ path = os.path.join(pa.template_basedir, power_type + ".template")
self.assertEqual(path, pa.path)
- def test_get_template(self):
- # get_template() should find and read the template file.
+ def test_template_basedir_defaults_to_local_dir(self):
+ self.configure_templates_dir()
+ self.assertEqual(
+ os.path.join(
+ os.path.dirname(os.path.dirname(__file__)), 'templates'),
+ PowerAction(POWER_TYPE.WAKE_ON_LAN).template_basedir)
+
+ def test_template_basedir_prefers_configured_value(self):
+ power_type = POWER_TYPE.WAKE_ON_LAN
+ template_name = '%s.template' % power_type
+ template = self.make_file(name=template_name)
+ template_dir = os.path.dirname(template)
+ self.configure_templates_dir(template_dir)
+ self.assertEqual(
+ template_dir,
+ PowerAction(POWER_TYPE.WAKE_ON_LAN).template_basedir)
+
+ def test_get_template_retrieves_template(self):
+ self.configure_templates_dir()
pa = PowerAction(POWER_TYPE.WAKE_ON_LAN)
template = pa.get_template()
self.assertIsInstance(template, ShellTemplate)
- with open(pa.path, "rb") as f:
- template_content = f.read()
- self.assertEqual(template_content, template.content)
+ self.assertThat(pa.path, FileContains(template.content))
+
+ def test_get_template_looks_for_template_in_template_basedir(self):
+ contents = factory.getRandomString()
+ power_type = POWER_TYPE.WAKE_ON_LAN
+ template_name = '%s.template' % power_type
+ template = self.make_file(name=template_name, contents=contents)
+ self.configure_templates_dir(os.path.dirname(template))
+ self.assertEqual(
+ contents,
+ PowerAction(power_type).get_template().content)
def test_render_template(self):
# render_template() should take a template string and substitue
@@ -83,6 +113,7 @@
"in file %s" % re.escape(template_name)))
def _create_template_file(self, template):
+ """Create a temporary template file with the given contents."""
return self.make_file("testscript.sh", template)
def run_action(self, path, **kwargs):
@@ -92,10 +123,8 @@
def test_execute(self):
# execute() should run the template through a shell.
-
- # Create a template in a temp dir.
- tempdir = self.make_dir()
- output_file = os.path.join(tempdir, "output")
+ output_file = self.make_file(
+ name='output', contents="(Output should go here)")
template = "echo working {{mac}} > {{outfile}}"
path = self._create_template_file(template)
@@ -103,8 +132,7 @@
self.assertThat(output_file, FileContains("working test\n"))
def test_execute_raises_PowerActionFail_when_script_fails(self):
- template = "this_is_not_valid_shell"
- path = self._create_template_file(template)
+ path = self._create_template_file("this_is_not_valid_shell")
exception = self.assertRaises(PowerActionFail, self.run_action, path)
self.assertEqual(
"ether_wake failed with return code 127", exception.message)
=== modified file 'src/provisioningserver/pxe/pxeconfig.py'
--- src/provisioningserver/pxe/pxeconfig.py 2012-06-26 10:22:48 +0000
+++ src/provisioningserver/pxe/pxeconfig.py 2012-06-28 07:46:21 +0000
@@ -84,7 +84,14 @@
@property
def template_basedir(self):
- return PXE_TEMPLATES_DIR
+ """Directory where PXE templates are stored."""
+ if PXE_TEMPLATES_DIR is None:
+ # The PXE templates are installed into the same location as this
+ # file, and also live in the same directory as this file in the
+ # source tree.
+ return os.path.join(os.path.dirname(__file__), 'templates')
+ else:
+ return PXE_TEMPLATES_DIR
def _validate_mac(self, mac):
# A MAC address should be of the form aa:bb:cc:dd:ee:ff with
=== modified file 'src/provisioningserver/pxe/tests/test_pxeconfig.py'
--- src/provisioningserver/pxe/tests/test_pxeconfig.py 2012-06-26 07:00:23 +0000
+++ src/provisioningserver/pxe/tests/test_pxeconfig.py 2012-06-28 07:46:21 +0000
@@ -15,9 +15,9 @@
import os
import re
-from celeryconfig import PXE_TEMPLATES_DIR
from maastesting.factory import factory
from maastesting.testcase import TestCase
+import provisioningserver.pxe.pxeconfig
from provisioningserver.pxe.pxeconfig import (
PXEConfig,
PXEConfigFail,
@@ -37,10 +37,16 @@
class TestPXEConfig(TestCase):
"""Tests for PXEConfig."""
+ def configure_templates_dir(self, path=None):
+ """Configure PXE_TEMPLATES_DIR to `path`."""
+ self.patch(
+ provisioningserver.pxe.pxeconfig, 'PXE_TEMPLATES_DIR', path)
+
def test_init_sets_up_paths(self):
pxeconfig = PXEConfig("armhf", "armadaxp")
- expected_template = os.path.join(PXE_TEMPLATES_DIR, "maas.template")
+ expected_template = os.path.join(
+ pxeconfig.template_basedir, 'maas.template')
expected_target = os.path.dirname(locate_tftp_path(
compose_config_path('armhf', 'armadaxp', 'default')))
self.assertEqual(expected_template, pxeconfig.template)
@@ -74,12 +80,36 @@
compose_config_path('armhf', 'armadaxp', '00-a1-b2-c3-e4-d5'))
self.assertEqual(expected_filename, pxeconfig.target_file)
- def test_get_template(self):
+ def test_template_basedir_defaults_to_local_dir(self):
+ self.configure_templates_dir()
+ arch = factory.make_name('arch')
+ self.assertEqual(
+ os.path.join(
+ os.path.dirname(os.path.dirname(__file__)), 'templates'),
+ PXEConfig(arch).template_basedir)
+
+ def test_template_basedir_prefers_configured_value(self):
+ temp_dir = self.make_dir()
+ self.configure_templates_dir(temp_dir)
+ arch = factory.make_name('arch')
+ self.assertEqual(
+ temp_dir,
+ PXEConfig(arch).template_basedir)
+
+ def test_get_template_retrieves_template(self):
+ self.configure_templates_dir()
pxeconfig = PXEConfig("i386")
template = pxeconfig.get_template()
self.assertIsInstance(template, tempita.Template)
self.assertThat(pxeconfig.template, FileContains(template.content))
+ def test_get_template_looks_for_template_in_template_basedir(self):
+ contents = factory.getRandomString()
+ template = self.make_file(name='maas.template', contents=contents)
+ self.configure_templates_dir(os.path.dirname(template))
+ arch = factory.make_name('arch')
+ self.assertEqual(contents, PXEConfig(arch).get_template().content)
+
def test_render_template(self):
pxeconfig = PXEConfig("i386")
template = tempita.Template("template: {{kernelimage}}")
