Merge lp:~terceiro/lava-dispatcher/expose-config-dirs into lp:lava-dispatcher

[Antonio Terceiro]

Refactor handling of configuration directories

The configuration search path is now unconditionally set to:

$HOME/.config/lava-dispatcher
${VIRTUAL_ENV}/etc/lava-dispatcher
${lava_dispatcher_root}/lava_dispatcher/default-config/lava-dispatcher

If the VIRTUAL_ENV environment variable is not set, then it will read
configuration from /etc/lava-dispatcher (as expected).

- The search path is now exposed to client code by the
  lava_dispatcher.config.search_path() function

- Each individual directory in the search path can be accessed by the variables
  user_config_path, system_config_path and default_config_path.

- Switched from /etc/xdg/lava-dispatcher to /etc/lava-dispatcher. Because of
  the previous state of the code, where a specific config_dir was always passed
  in and there was no way of overriding it, /etc/xdg/lava-dispatcher was never
  actually used by anybody, so the risk of this change is zero.

- Removed passing around of configuration directory.

- --config-dir switch in command line tools was kept and has the same behaviour
  as before. If a custom config dir is passed, both user and system-wide
  directories are ignored, and only the custom directory and the hardcoded
  defaults are used.

[Milo Casagrande (milo)]

Thanks Antonio for working on this!
FWIW, +1 from me, it looks good.

One note only: it might be good to update documentation with the new behavior. There are references in doc/QUICKSTART (that is "linked" from the README), plus I guess the docs that will appear on lava.readthedocs.org. I can manage to do that in case, so I'll start to take a look around this code base.

[Antonio Terceiro (terceiro)]

On Wed, Jun 12, 2013 at 09:30:17AM -0000, Milo Casagrande wrote:
> Thanks Antonio for working on this!
> FWIW, +1 from me, it looks good.
>
> One note only: it might be good to update documentation with the new
> behavior. There are references in doc/QUICKSTART (that is "linked"
> from the README), plus I guess the docs that will appear on
> lava.readthedocs.org. I can manage to do that in case, so I'll start
> to take a look around this code base.

Thanks - I just updated doc/QUICKSTART. I couldn't find any other
reference to /etc/xdg elsewhere.

--
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org

[Neil Williams (codehelp)]

I've tested this with my git packaging branch which relies on the settings in /etc/lava-dispatcher/lava-dispatcher.conf and it fixes the problem and correctly uses the values in /etc/ in preference to those in /usr/share/lava_dispatcher/default-config/.

git://git.linaro.org/people/neilwilliams/lava-dispatcher.git
(multinode branch)

Thanks for the fix, Antonio.

[Neil Williams (codehelp)]

review: Approve

=== modified file 'lava/dispatcher/commands.py'
--- lava/dispatcher/commands.py	2013-01-23 22:28:49 +0000
+++ lava/dispatcher/commands.py	2013-06-11 20:55:28 +0000
@@ -1,3 +1,4 @@
+import argparse
 import json
 import logging
 import os
@@ -7,26 +8,24 @@
 from lava.tool.command import Command
 from lava.tool.errors import CommandError
 
+import lava_dispatcher.config
 from lava_dispatcher.config import get_config, get_device_config, get_devices
 from lava_dispatcher.job import LavaTestJob, validate_job_data
 
 
+class SetUserConfigDirAction(argparse.Action):
+    def __call__(self, parser, namespace, value, option_string=None):
+        lava_dispatcher.config.custom_config_path = value
+
+
 class DispatcherCommand(Command):
     @classmethod
     def register_arguments(cls, parser):
         super(DispatcherCommand, cls).register_arguments(parser)
-        # When we're working inside a virtual environment use venv-relative
-        # configuration directory. This works well with lava-deployment-tool
-        # and the directory layout it currently provides but will need to be
-        # changed for codeline support.
-        if "VIRTUAL_ENV" in os.environ:
-            default_config_dir = os.path.join(
-                os.environ["VIRTUAL_ENV"], "etc", "lava-dispatcher")
-        else:
-            default_config_dir = None
         parser.add_argument(
             "--config-dir",
-            default=default_config_dir,
+            default=None,
+            action=SetUserConfigDirAction,
             help="Configuration directory override (currently %(default)s")
 
 
@@ -35,7 +34,7 @@
     Lists all the configured devices in this LAVA instance.
     """
     def invoke(self):
-        for d in get_devices(self.args.config_dir):
+        for d in get_devices():
             print d.hostname
 
 
@@ -88,7 +87,7 @@
         FORMAT = '<LAVA_DISPATCHER>%(asctime)s %(levelname)s: %(message)s'
         DATEFMT = '%Y-%m-%d %I:%M:%S %p'
         logging.basicConfig(format=FORMAT, datefmt=DATEFMT)
-        config = get_config(self.args.config_dir)
+        config = get_config()
         logging.root.setLevel(config.logging_level)
 
         # Set process id if job-id was passed to dispatcher
@@ -139,7 +138,7 @@
     @property
     def device_config(self):
         try:
-            return get_device_config(self.args.device, self.args.config_dir)
+            return get_device_config(self.args.device)
         except Exception:
             raise CommandError("no such device: %s" % self.args.device)
 

=== modified file 'lava_dispatcher/config.py'
--- lava_dispatcher/config.py	2013-06-05 21:43:24 +0000
+++ lava_dispatcher/config.py	2013-06-11 20:55:28 +0000
@@ -152,30 +152,40 @@
 
 class DispatcherConfig(object):
 
-    def __init__(self, cp, config_dir):
+    def __init__(self, cp):
         self.cp = cp
-        self.config_dir = config_dir
 
     for option in DispatcherSchema().options():
         locals()[option.name] = OptionDescriptor(option.name)
 
 
-default_config_path = os.path.join(
-    os.path.dirname(__file__), 'default-config')
-
-
-def load_config_paths(name, config_dir):
-    if config_dir is None:
-        paths = [
-            os.path.join(path, name) for path in [
-                os.path.expanduser("~/.config"),
-                "/etc/xdg",
-                default_config_path]]
+user_config_path = os.path.expanduser("~/.config/lava-dispatcher")
+
+if "VIRTUAL_ENV" in os.environ:
+    system_config_path = os.path.join(os.environ["VIRTUAL_ENV"],
+                                      "etc/lava-dispatcher")
+else:
+    system_config_path = "/etc/lava-dispatcher"
+
+deprecated_system_config_path = "/etc/xdg/lava-dispatcher"
+
+default_config_path = os.path.join(os.path.dirname(__file__),
+                                   'default-config/lava-dispatcher')
+
+custom_config_path = None
+
+def search_path():
+    if custom_config_path:
+        return [
+            custom_config_path,
+            default_config_path,
+        ]
     else:
-        paths = [config_dir, os.path.join(default_config_path, name)]
-    for path in paths:
-        if os.path.isdir(path):
-            yield path
+        return [
+            user_config_path,
+            system_config_path,
+            default_config_path,
+        ]
 
 
 def _read_into(path, cp):
@@ -186,7 +196,7 @@
     cp.readfp(s)
 
 
-def _get_config(name, config_dir, cp):
+def _get_config(name, cp):
     """Read a config file named name + '.conf'.
 
     This checks and loads files from the source tree, site wide location and
@@ -194,7 +204,7 @@
     settings which override source settings.
     """
     config_files = []
-    for directory in load_config_paths('lava-dispatcher', config_dir):
+    for directory in search_path():
         path = os.path.join(directory, '%s.conf' % name)
         if os.path.exists(path):
             config_files.append(path)
@@ -207,13 +217,13 @@
     return cp
 
 
-def get_config(config_dir):
+def get_config():
     cp = parser.SchemaConfigParser(DispatcherSchema())
-    _get_config("lava-dispatcher", config_dir, cp)
+    _get_config("lava-dispatcher", cp)
     valid, report = cp.is_valid(report=True)
     if not valid:
         logging.warning("dispatcher config is not valid:\n    %s", '\n    '.join(report))
-    return DispatcherConfig(cp, config_dir)
+    return DispatcherConfig(cp)
 
 
 def _hack_boot_options(scp):
@@ -243,19 +253,19 @@
     return scrubbed
 
 
-def get_device_config(name, config_dir):
+def get_device_config(name):
     # We read the device config once to get the device type, then we start
     # again and read device-defaults, device-types/$device-type and
     # devices/$device in that order.
     initial_config = ConfigParser()
-    _get_config("devices/%s" % name, config_dir, initial_config)
+    _get_config("devices/%s" % name, initial_config)
 
     real_device_config = parser.SchemaConfigParser(DeviceSchema())
-    _get_config("device-defaults", config_dir, real_device_config)
+    _get_config("device-defaults", real_device_config)
     _get_config(
         "device-types/%s" % initial_config.get('__main__', 'device_type'),
-        config_dir, real_device_config)
-    _get_config("devices/%s" % name, config_dir, real_device_config)
+        real_device_config)
+    _get_config("devices/%s" % name, real_device_config)
     real_device_config.set("__main__", "hostname", name)
     _hack_boot_options(real_device_config)
     valid, report = real_device_config.is_valid(report=True)
@@ -269,10 +279,19 @@
     return DeviceConfig(real_device_config)
 
 
-def get_devices(config_dir):
+def get_devices():
     devices = []
-    devices_dir = os.path.join(config_dir, 'devices')
-    for d in os.listdir(devices_dir):
-        d = os.path.splitext(d)[0]
-        devices.append(get_device_config(d, config_dir))
+    for config_dir in search_path():
+        devices_dir = os.path.join(config_dir, 'devices')
+        if os.path.isdir(devices_dir):
+            for d in os.listdir(devices_dir):
+                d = os.path.splitext(d)[0]
+                devices.append(get_device_config(d))
     return devices
+
+def get_config_file(config_file):
+    for config_dir in search_path():
+        config_file_path = os.path.join(config_dir, config_file)
+        if os.path.exists(config_file_path):
+            return config_file_path
+    return None

=== modified file 'lava_dispatcher/context.py'
--- lava_dispatcher/context.py	2013-04-02 16:16:18 +0000
+++ lava_dispatcher/context.py	2013-06-11 20:55:28 +0000
@@ -94,8 +94,7 @@
         self.job_data = job_data
         self.output = Outputter(output_dir)
         self.logfile_read = self.output.logfile_read
-        self.device_config = get_device_config(
-            target, dispatcher_config.config_dir)
+        self.device_config = get_device_config(target)
         self._client = TargetBasedClient(self, self.device_config)
         self.test_data = LavaTestData()
         self.oob_file = oob_file

=== modified file 'lava_dispatcher/downloader.py'
--- lava_dispatcher/downloader.py	2012-12-18 19:50:48 +0000
+++ lava_dispatcher/downloader.py	2013-06-11 20:55:28 +0000
@@ -33,6 +33,7 @@
 import zlib
 
 from tempfile import mkdtemp
+from lava_dispatcher.config import get_config_file
 from lava_dispatcher.utils import rmtree
 
 
@@ -122,8 +123,8 @@
     '''allows the downloader to override a URL so that something like:
      http://blah/ becomes file://localhost/blah
     '''
-    mappings = '%s/urlmappings.txt' % context.config.config_dir
-    if os.path.exists(mappings):
+    mappings = get_config_file('urlmappings.txt')
+    if mappings:
         newurl = url
         with open(mappings, 'r') as f:
             for line in f.readlines():