launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10417
[Merge] lp:~allenap/maas/pxe-config-slimming into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/pxe-config-slimming into lp:maas with lp:~allenap/maas/clean-up-locate-tftp-path as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/maas/pxe-config-slimming/+merge/117346
I had been looking at where to properly put the kernel and initrd
options (bug 1030878) and I stumbled across the PXEConfig class. ISTR
seeing this once before. I think it had a fair bit of YAGNI before,
but now, with other changes that have come along since, it's mostly
YAGNI. I've trimmed all the fat off, leaving something that doesn't
make my brain swap things out when parsing it :)
The diffstat says it all really:
7 files changed, 67 insertions(+), 179 deletions(-)
--
https://code.launchpad.net/~allenap/maas/pxe-config-slimming/+merge/117346
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/pxe-config-slimming into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-07-30 15:42:41 +0000
+++ src/maasserver/api.py 2012-07-30 21:37:23 +0000
@@ -133,10 +133,7 @@
from piston.models import Token
from piston.resource import Resource
from piston.utils import rc
-from provisioningserver.pxe.pxeconfig import (
- PXEConfig,
- PXEConfigFail,
- )
+from provisioningserver.pxe.config import render_pxe_config
dispatch_methods = {
@@ -1078,35 +1075,38 @@
def pxeconfig(request):
"""Get the PXE configuration given a node's details.
- :param arch: Main machine architecture.
- :param subarch: Sub-architecture, or "generic" if there is none.
:param mac: MAC address to produce a boot configuration for. This
parameter is optional. If it is not given, the configuration
will be the "default" one which boots into an enlistment image.
- :param menu_title: Title that the node should show in its PXE menu.
+ :param title: Title that the node should show in its PXE menu.
:param kernel: TFTP path to the kernel image that is to be booted.
:param initrd: TFTP path to the initrd that is to be booted.
:param append: Additional parameters to append to the kernel command
line.
+
+ In addition, the following parameters are expected, but are not used:
+
+ :param arch: Main machine architecture.
+ :param subarch: Sub-architecture, or "generic" if there is none.
"""
- menu_title = get_mandatory_param(request.GET, 'menu_title')
+ title = get_mandatory_param(request.GET, 'title')
kernel = get_mandatory_param(request.GET, 'kernel')
initrd = get_mandatory_param(request.GET, 'initrd')
append = get_mandatory_param(request.GET, 'append')
+ mac = request.GET.get('mac', None)
+
+ # The following two parameters - arch and subarch - typically must be
+ # supplied, but are unused right now.
arch = get_mandatory_param(request.GET, 'arch')
subarch = request.GET.get('subarch', 'generic')
- mac = request.GET.get('mac', None)
- config = PXEConfig(arch, subarch)
+ arch, subarch # Suppress lint warnings.
# In addition to the "append" parameter, also add a URL for the
# node's preseed to the kernel command line.
append = "%s %s" % (append, compose_preseed_kernel_opt(mac))
- try:
- return HttpResponse(
- config.get_config(
- menu_title=menu_title, kernel=kernel, initrd=initrd,
- append=append),
- content_type="text/plain; charset=utf-8")
- except PXEConfigFail as e:
- raise ValidationError(e.message)
+ return HttpResponse(
+ render_pxe_config(
+ title=title, kernel=kernel,
+ initrd=initrd, append=append),
+ content_type="text/plain; charset=utf-8")
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-07-30 15:42:41 +0000
+++ src/maasserver/tests/test_api.py 2012-07-30 21:37:23 +0000
@@ -2245,7 +2245,7 @@
'arch': "armhf",
'subarch': "armadaxp",
'mac': factory.make_mac_address().mac_address,
- 'menu_title': factory.make_name("Menu"),
+ 'title': factory.make_name("Menu"),
'kernel': factory.make_name("/my/kernel"),
'initrd': factory.make_name("/my/initrd"),
'append': factory.make_name("append"),
@@ -2284,7 +2284,7 @@
'arch': httplib.BAD_REQUEST,
'subarch': httplib.OK,
'mac': httplib.OK,
- 'menu_title': httplib.BAD_REQUEST,
+ 'title': httplib.BAD_REQUEST,
'kernel': httplib.BAD_REQUEST,
'initrd': httplib.BAD_REQUEST,
'append': httplib.BAD_REQUEST,
=== renamed file 'src/provisioningserver/pxe/pxeconfig.py' => 'src/provisioningserver/pxe/config.py'
--- src/provisioningserver/pxe/pxeconfig.py 2012-07-26 07:54:27 +0000
+++ src/provisioningserver/pxe/config.py 2012-07-30 21:37:23 +0000
@@ -16,79 +16,27 @@
__metaclass__ = type
__all__ = [
- 'PXEConfig',
- 'PXEConfigFail',
+ 'render_pxe_config',
]
-import os
+from os import path
-from celeryconfig import PXE_TEMPLATES_DIR
import tempita
-class PXEConfigFail(Exception):
- """Raised if there's a problem with a PXE config."""
-
-
-class PXEConfig:
- """A PXE configuration file.
-
- Encapsulation of PXE config templates and parameter substitution.
-
- :param arch: The architecture to write a configuration for, e.g. i386.
- :type arch: string
- :param subarch: Sub-architecture. Only needed for architectures that
- have sub-architectures, such as ARM; other architectures use
- a sub-architecture of "generic" (which is the default).
- :type subarch: string
-
- :raises PXEConfigFail: if there's a problem substituting the template
- parameters.
-
- Use this class by instantiating with parameters that define its location:
-
- >>> pxeconfig = PXEConfig("armhf", "armadaxp")
-
- and then produce a configuration file with:
-
- >>> pxeconfig.get_config(
- ... menu_title="Booting", kernel="/my/kernel", initrd="/my/initrd",
- append="auto url=blah")
+template_dir = path.dirname(__file__)
+template_filename = path.join(template_dir, "config.template")
+template = tempita.Template.from_filename(template_filename, encoding="UTF-8")
+
+
+def render_pxe_config(title, kernel, initrd, append):
+ """Render a PXE configuration file as a unicode string.
+
+ :param title: Title that the node should show on its boot menu.
+ :param kernel: TFTP path to the kernel image to boot.
+ :param initrd: TFTP path to the initrd file to boot from.
+ :param append: Additional kernel parameters.
"""
-
- def __init__(self, arch, subarch='generic'):
- self.template = os.path.join(self.template_basedir, "maas.template")
-
- @property
- def template_basedir(self):
- """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 get_template(self):
- with open(self.template, "r") as f:
- return tempita.Template(f.read(), name=self.template)
-
- def render_template(self, template, **kwargs):
- try:
- return template.substitute(kwargs)
- except NameError as error:
- raise PXEConfigFail(*error.args)
-
- def get_config(self, **kwargs):
- """Return this PXE config file as a unicode string.
-
- :param menu_title: Title that the node should show on its boot menu.
- :param kernel: TFTP path to the kernel image to boot.
- :param initrd: TFTP path to the initrd file to boot from.
- :param append: Additional parameters to append to the kernel
- command line.
- """
- template = self.get_template()
- return self.render_template(template, **kwargs)
+ return template.substitute(
+ title=title, kernel=kernel, initrd=initrd, append=append)
=== renamed file 'src/provisioningserver/pxe/templates/maas.template' => 'src/provisioningserver/pxe/config.template'
--- src/provisioningserver/pxe/templates/maas.template 2012-07-26 07:54:27 +0000
+++ src/provisioningserver/pxe/config.template 2012-07-30 21:37:23 +0000
@@ -1,6 +1,6 @@
DEFAULT menu
PROMPT 0
-MENU TITLE {{menu_title}}
+MENU TITLE {{title}}
TIMEOUT 1
ONTIMEOUT execute
=== removed directory 'src/provisioningserver/pxe/templates'
=== renamed file 'src/provisioningserver/pxe/tests/test_pxeconfig.py' => 'src/provisioningserver/pxe/tests/test_config.py'
--- src/provisioningserver/pxe/tests/test_pxeconfig.py 2012-07-26 07:54:27 +0000
+++ src/provisioningserver/pxe/tests/test_config.py 2012-07-30 21:37:23 +0000
@@ -1,7 +1,7 @@
# Copyright 2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-"""Tests for `provisioningserver.pxe`."""
+"""Tests for `provisioningserver.pxe.config`."""
from __future__ import (
absolute_import,
@@ -12,94 +12,34 @@
__metaclass__ = type
__all__ = []
-import os
-import re
-
from maastesting.factory import factory
from maastesting.testcase import TestCase
-import provisioningserver.pxe.pxeconfig
-from provisioningserver.pxe.pxeconfig import (
- PXEConfig,
- PXEConfigFail,
- )
-import tempita
+from provisioningserver.pxe.config import render_pxe_config
from testtools.matchers import (
- FileContains,
- MatchesRegex,
+ Contains,
+ IsInstance,
+ StartsWith,
)
-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_template_path(self):
- pxeconfig = PXEConfig(factory.make_name('arch'))
- self.assertEqual(
- os.path.join(pxeconfig.template_basedir, 'maas.template'),
- PXEConfig("armhf", "armadaxp").template)
-
- 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: {{kernel}}")
- rendered = pxeconfig.render_template(template, kernel="myimage")
- self.assertEqual("template: myimage", rendered)
-
- def test_render_template_raises_PXEConfigFail(self):
- # If not enough arguments are supplied to fill in template
- # variables then a PXEConfigFail is raised.
- pxeconfig = PXEConfig("i386")
- template_name = factory.getRandomString()
- template = tempita.Template(
- "template: {{kernel}}", name=template_name)
- exception = self.assertRaises(
- PXEConfigFail, pxeconfig.render_template, template)
- self.assertThat(
- exception.message, MatchesRegex(
- "name 'kernel' is not defined at line \d+ column \d+ "
- "in file %s" % re.escape(template_name)))
-
- def test_get_config_returns_config(self):
- pxeconfig = PXEConfig("armhf", "armadaxp")
- template = pxeconfig.get_template()
- expected = pxeconfig.render_template(
- template, menu_title="Title", kernel="/my/kernel",
- initrd="/my/initrd", append="append")
-
- self.assertEqual(
- pxeconfig.get_config(
- menu_title="Title", kernel="/my/kernel", initrd="/my/initrd",
- append="append"),
- expected)
+class TestRenderPXEConfig(TestCase):
+ """Tests for `provisioningserver.pxe.config.render_pxe_config`."""
+
+ def test_render(self):
+ # Given the right configuration options, the PXE configuration is
+ # correctly rendered.
+ options = {
+ "title": factory.make_name("title"),
+ "kernel": factory.make_name("kernel"),
+ "initrd": factory.make_name("initrd"),
+ "append": factory.make_name("append"),
+ }
+ output = render_pxe_config(**options)
+ # The output is always a Unicode string.
+ self.assertThat(output, IsInstance(unicode))
+ # The template has rendered without error. PXELINUX configurations
+ # typically start with a DEFAULT line.
+ self.assertThat(output, StartsWith("DEFAULT "))
+ # All of the values put in are included somewhere in the output.
+ for value in options.values():
+ self.assertThat(output, Contains(value))
=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py 2012-07-27 20:25:52 +0000
+++ src/provisioningserver/tests/test_tftp.py 2012-07-30 21:37:23 +0000
@@ -133,12 +133,12 @@
mac = factory.getRandomMACAddress(b"-")
kernel = factory.make_name("kernel").encode("ascii")
initrd = factory.make_name("initrd").encode("ascii")
- menu_title = factory.make_name("menu-title").encode("ascii")
+ title = factory.make_name("menu-title").encode("ascii")
append = factory.make_name("append").encode("ascii")
backend_url = b"http://example.com/?"; + urlencode({
b"kernel": kernel,
b"initrd": initrd,
- b"menu_title": menu_title,
+ b"title": title,
b"append": append,
})
backend = TFTPBackend(self.make_dir(), backend_url)
@@ -153,7 +153,7 @@
("initrd", initrd),
("arch", arch),
("subarch", subarch),
- ("menu_title", menu_title),
+ ("title", title),
("mac", mac),
]
self.assertItemsEqual(query_expected, query)
=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py 2012-07-30 09:43:11 +0000
+++ src/provisioningserver/tftp.py 2012-07-30 21:37:23 +0000
@@ -110,7 +110,7 @@
"""
# TODO: update query defaults.
query = {
- b"menu_title": b"",
+ b"title": b"",
b"kernel": b"",
b"initrd": b"",
b"append": b"",
