launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/default-config-file into lp:maas with lp:~allenap/maas/remove-obsolete-generate-enlistment-pxe as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/default-config-file/+merge/116379

Fixes a TODO in lp:~allenap/maas/tftp-path-in-pserv. This defines the logic for figuring out which provisioning configuration file to load in one place, in a class property (i.e. a property defined on a metaclass), Config.DEFAULT_FILENAME.
-- 
https://code.launchpad.net/~allenap/maas/default-config-file/+merge/116379
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/default-config-file into lp:maas.

=== modified file 'src/maas/development.py'
--- src/maas/development.py	2012-07-23 21:28:19 +0000
+++ src/maas/development.py	2012-07-23 21:28:19 +0000
@@ -21,6 +21,7 @@
     settings,
     )
 from metadataserver.address import guess_server_address
+import provisioningserver.config
 
 # We expect the following settings to be overridden. They are mentioned here
 # to silence lint warnings.
@@ -98,8 +99,8 @@
 COMMISSIONING_SCRIPT = os.path.join(
     DEV_ROOT_DIRECTORY, 'etc/maas/commissioning-user-data')
 
-PROVISIONING_SETTINGS = abspath("etc/pserv.yaml")
-
+# Override the default provisioning config filename.
+provisioningserver.config.Config.DEFAULT_FILENAME = abspath("etc/pserv.yaml")
 
 # Set up celery to use the demo settings.
 os.environ['CELERY_CONFIG_MODULE'] = 'democeleryconfig'

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-07-23 21:28:19 +0000
+++ src/maas/settings.py	2012-07-06 10:12:25 +0000
@@ -307,11 +307,5 @@
     "/usr/share/maas/preseeds",
     )
 
-# Settings used for provisioning.
-# TODO: un-cargo-cult this from provisioningserver.utils.MainScript.
-PROVISIONING_SETTINGS = os.environ.get(
-    "MAAS_PROVISIONING_SETTINGS", "/etc/maas/pserv.yaml")
-
-
 # Allow the user to override settings in maas_local_settings.
 import_local_settings()

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-07-23 21:28:19 +0000
+++ src/maasserver/api.py	2012-07-23 21:28:19 +0000
@@ -1085,9 +1085,7 @@
     :param kernelimage: The path to the kernel in the TFTP server
     :param append: Kernel parameters to append.
     """
-    provisioning_config = (
-        provisioningserver.config.Config.load_from_cache(
-            settings.PROVISIONING_SETTINGS))
+    provisioning_config = provisioningserver.config.Config.load_from_cache()
     menutitle = get_mandatory_param(request.GET, 'menutitle')
     kernelimage = get_mandatory_param(request.GET, 'kernelimage')
     append = get_mandatory_param(request.GET, 'append')

=== modified file 'src/provisioningserver/config.py'
--- src/provisioningserver/config.py	2012-07-23 21:28:19 +0000
+++ src/provisioningserver/config.py	2012-07-23 21:28:19 +0000
@@ -15,10 +15,12 @@
     ]
 
 from getpass import getuser
+from os import environ
 from os.path import abspath
 from threading import RLock
 
 from formencode import Schema
+from formencode.declarative import DeclarativeMeta
 from formencode.validators import (
     Int,
     RequireIfPresent,
@@ -79,9 +81,37 @@
         )
 
 
+class ConfigMeta(DeclarativeMeta):
+    """Metaclass for the root configuration schema."""
+
+    def _get_default_filename(cls):
+        # Get the configuration filename from the environment. Failing that,
+        # return a hard-coded default.
+        return environ.get(
+            "MAAS_PROVISIONING_SETTINGS",
+            "/etc/maas/pserv.yaml")
+
+    def _set_default_filename(cls, filename):
+        # Set the configuration filename in the environment.
+        environ["MAAS_PROVISIONING_SETTINGS"] = filename
+
+    def _delete_default_filename(cls):
+        # Remove any setting of the configuration filename from the
+        # environment.
+        environ.pop("MAAS_PROVISIONING_SETTINGS", None)
+
+    DEFAULT_FILENAME = property(
+        _get_default_filename, _set_default_filename,
+        _delete_default_filename, doc=(
+            "The default config file to load. Refers to "
+            "MAAS_PROVISIONING_SETTINGS in the environment."))
+
+
 class Config(Schema):
     """Configuration validator."""
 
+    __metaclass__ = ConfigMeta
+
     if_key_missing = None
 
     interface = String(if_empty=b"", if_missing=b"127.0.0.1")
@@ -100,8 +130,10 @@
         return cls.to_python(yaml.safe_load(stream))
 
     @classmethod
-    def load(cls, filename):
+    def load(cls, filename=None):
         """Load a YAML configuration from `filename` and validate."""
+        if filename is None:
+            filename = cls.DEFAULT_FILENAME
         with open(filename, "rb") as stream:
             return cls.parse(stream)
 
@@ -109,11 +141,13 @@
     _cache_lock = RLock()
 
     @classmethod
-    def load_from_cache(cls, filename):
+    def load_from_cache(cls, filename=None):
         """Load or return a previously loaded configuration.
 
         This is thread-safe, so is okay to use from Django, for example.
         """
+        if filename is None:
+            filename = cls.DEFAULT_FILENAME
         filename = abspath(filename)
         with cls._cache_lock:
             if filename not in cls._cache:

=== modified file 'src/provisioningserver/tests/test_config.py'
--- src/provisioningserver/tests/test_config.py	2012-07-23 21:28:19 +0000
+++ src/provisioningserver/tests/test_config.py	2012-07-23 21:28:19 +0000
@@ -17,6 +17,7 @@
 import os
 from textwrap import dedent
 
+from fixtures import EnvironmentVariableFixture
 import formencode
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
@@ -63,6 +64,39 @@
         self.exercise_fixture(fixture)
 
 
+class TestConfig_DEFAULT_FILENAME(TestCase):
+    """Tests for `provisioningserver.config.Config.DEFAULT_FILENAME`."""
+
+    def setUp(self):
+        super(TestConfig_DEFAULT_FILENAME, self).setUp()
+        # Start with a clean environment every time.
+        fixture = EnvironmentVariableFixture("MAAS_PROVISIONING_SETTINGS")
+        self.useFixture(fixture)
+
+    def test_get_with_environment_empty(self):
+        self.assertEqual("/etc/maas/pserv.yaml", Config.DEFAULT_FILENAME)
+
+    def test_get_with_environment_set(self):
+        dummy_filename = factory.make_name("config")
+        fixture = EnvironmentVariableFixture(
+            "MAAS_PROVISIONING_SETTINGS", dummy_filename)
+        self.useFixture(fixture)
+        self.assertEqual(dummy_filename, Config.DEFAULT_FILENAME)
+
+    def test_set(self):
+        dummy_filename = factory.make_name("config")
+        Config.DEFAULT_FILENAME = dummy_filename
+        self.assertEqual(dummy_filename, Config.DEFAULT_FILENAME)
+
+    def test_delete(self):
+        Config.DEFAULT_FILENAME = factory.make_name("config")
+        del Config.DEFAULT_FILENAME
+        # The filename reverts; see test_get_with_environment_empty.
+        self.assertEqual("/etc/maas/pserv.yaml", Config.DEFAULT_FILENAME)
+        # The delete does not fail when called multiple times.
+        del Config.DEFAULT_FILENAME
+
+
 class TestConfig(TestCase):
     """Tests for `provisioningserver.config.Config`."""
 

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-07-23 21:28:19 +0000
+++ src/provisioningserver/utils.py	2012-07-23 21:28:19 +0000
@@ -23,10 +23,7 @@
 from argparse import ArgumentParser
 from functools import wraps
 import os
-from os import (
-    fdopen,
-    environ,
-    )
+from os import fdopen
 from pipes import quote
 import signal
 from subprocess import CalledProcessError
@@ -34,6 +31,7 @@
 import tempfile
 from time import time
 
+from provisioningserver.config import Config
 import tempita
 from twisted.internet.defer import maybeDeferred
 from zope.interface.interface import Method
@@ -258,6 +256,4 @@
         self.parser.add_argument(
             "-c", "--config-file", metavar="FILENAME",
             help="Configuration file to load [%(default)s].",
-            default=environ.get(
-                "MAAS_PROVISIONING_SETTINGS",
-                "/etc/maas/pserv.yaml"))
+            default=Config.DEFAULT_FILENAME)