Options: ensure <repository> macros are always fixed up

This fixes several cases in which <repository> macros would not be
properly processed: options that are not added to the parser yet when
early options are parsed; and config file options whose default value
is used.

5 files changed, 130 insertions, 46 deletions

diff --git a/src/lib/Bcfg2/Options/Options.py b/src/lib/Bcfg2/Options/Options.py
index bd1a72fc7..36148c279 100644
--- a/src/lib/Bcfg2/Options/Options.py
+++ b/src/lib/Bcfg2/Options/Options.py
@@ -17,7 +17,7 @@ from Bcfg2.Compat import ConfigParser
 __all__ = ["Option", "BooleanOption", "PathOption", "PositionalArgument",
            "_debug"]
 
-unit_test = False
+unit_test = False  # pylint: disable=C0103
 
 
 def _debug(msg):
@@ -46,7 +46,7 @@ def _get_action_class(action_name):
     on.  So we just instantiate a dummy parser, add a dummy argument,
     and determine the class that way. """
     if (isinstance(action_name, type) and
-        issubclass(action_name, argparse.Action)):
+            issubclass(action_name, argparse.Action)):
         return action_name
     if action_name not in _action_map:
         action = argparse.ArgumentParser().add_argument(action_name,
@@ -159,9 +159,10 @@ class Option(object):
             sources.append("%s.%s" % self.cf)
         if self.env:
             sources.append("$" + self.env)
-        spec = ["sources=%s" % sources, "default=%s" % self.default]
-        spec.append("%d parsers" % (len(self.parsers)))
-        return 'Option(%s: %s)' % (self.dest, ", ".join(spec))
+        spec = ["sources=%s" % sources, "default=%s" % self.default,
+                "%d parsers" % len(self.parsers)]
+        return '%s(%s: %s)' % (self.__class__.__name__,
+                               self.dest, ", ".join(spec))
 
     def list_options(self):
         """ List options contained in this option.  This exists to
@@ -228,7 +229,7 @@ class Option(object):
                 rv = self._type_func(self.get_config_value(cfp))
             except (ConfigParser.NoSectionError, ConfigParser.NoOptionError):
                 rv = None
-        _debug("Setting %s from config file(s): %s" % (self, rv))
+        _debug("Getting value of %s from config file(s): %s" % (self, rv))
         return rv
 
     def get_config_value(self, cfp):
@@ -326,12 +327,11 @@ class Option(object):
 
 
 class PathOption(Option):
-    """ Shortcut for options that expect a path argument. Uses
-    :meth:`Bcfg2.Options.Types.path` to transform the argument into a
-    canonical path.
+    """Shortcut for options that expect a path argument.
 
-    The type of a path option can also be overridden to return a
-    file-like object. For example:
+    Uses :meth:`Bcfg2.Options.Types.path` to transform the argument
+    into a canonical path. The type of a path option can also be
+    overridden to return a file-like object. For example:
 
     .. code-block:: python
 
@@ -339,25 +339,50 @@ class PathOption(Option):
             Bcfg2.Options.PathOption(
                 "--input", type=argparse.FileType('r'),
                 help="The input file")]
+
+    PathOptions also do translation of ``<repository>`` macros on the
+    fly. It's done this way instead of just fixing up all values at
+    the end of parsing because macro expansion needs to be done before
+    path canonicalization and other stuff.
     """
+    repository = None
 
     def __init__(self, *args, **kwargs):
         self._original_type = kwargs.pop('type', lambda x: x)
         kwargs['type'] = self._type
         kwargs.setdefault('metavar', '<path>')
         Option.__init__(self, *args, **kwargs)
-        self.repository = None
 
     def early_parsing_hook(self, early_opts):
-        self.repository = early_opts.repository
+        if hasattr(early_opts, "repository"):
+            if self.__class__.repository is None:
+                _debug("Setting repository to %s for PathOptions" %
+                       early_opts.repository)
+                self.__class__.repository = early_opts.repository
+            else:
+                _debug("Repository is already set for %s" % self.__class__)
+
+    def _get_default(self):
+        """ Getter for the ``default`` property """
+        if self.__class__.repository is None or self._default is None:
+            return self._default
+        else:
+            return self._default.replace("<repository>",
+                                         self.__class__.repository)
+
+    default = property(_get_default, Option._set_default)
 
     def _type(self, value):
         """Type function that fixes up <repository> macros."""
-        if self.repository is None:
-            rv = value
+        if self.__class__.repository is None:
+            _debug("Cannot fix up <repository> macros yet for %s" % self)
+            return value
         else:
-            rv = value.replace("<repository>", self.repository)
-        return self._original_type(Types.path(rv))
+            rv = self._original_type(Types.path(
+                value.replace("<repository>", self.__class__.repository)))
+            _debug("Fixing up <repository> macros in %s: %s -> %s" %
+                   (self, value, rv))
+            return rv
 
 
 class _BooleanOptionAction(argparse.Action):
diff --git a/src/lib/Bcfg2/Options/Parser.py b/src/lib/Bcfg2/Options/Parser.py
index ec470f8d3..6715d90f9 100644
--- a/src/lib/Bcfg2/Options/Parser.py
+++ b/src/lib/Bcfg2/Options/Parser.py
@@ -168,6 +168,8 @@ class Parser(argparse.ArgumentParser):
                                (opt, value))
                         action(self, self.namespace, value)
                 else:
+                    _debug("Setting config file-only option %s to %s" %
+                           (opt, value))
                     setattr(self.namespace, opt.dest, value)
 
     def _finalize(self):
@@ -191,6 +193,53 @@ class Parser(argparse.ArgumentParser):
                 _debug("Deleting %s" % attr)
                 delattr(self.namespace, attr)
 
+    def _parse_early_options(self):
+        """Parse early options.
+
+        Early options are options that need to be parsed before other
+        options for some reason. These fall into two basic cases:
+
+        1. Database options, which need to be parsed so that Django
+           modules can be imported, since Django configuration is all
+           done at import-time;
+        2. The repository (``-Q``) option, so that ``<repository>``
+           macros in other options can be resolved.
+        """
+        _debug("Option parsing phase 2: Parse early options")
+        early_opts = argparse.Namespace()
+        early_parser = Parser(add_help=False, namespace=early_opts,
+                              early=True)
+
+        # add the repo option so we can resolve <repository>
+        # macros
+        early_parser.add_options([repository])
+
+        early_components = []
+        for component in self.components:
+            if getattr(component, "parse_first", False):
+                early_components.append(component)
+                early_parser.add_component(component)
+        early_parser.parse(self.argv)
+
+        _debug("Fixing up <repository> macros in early options")
+        for attr_name in dir(early_opts):
+            if not attr_name.startswith("_"):
+                attr = getattr(early_opts, attr_name)
+                if hasattr(attr, "replace"):
+                    setattr(early_opts, attr_name,
+                            attr.replace("<repository>",
+                                         early_opts.repository))
+
+        _debug("Early parsing complete, calling hooks")
+        for component in early_components:
+            if hasattr(component, "component_parsed_hook"):
+                _debug("Calling component_parsed_hook on %s" % component)
+                getattr(component, "component_parsed_hook")(early_opts)
+        _debug("Calling early parsing hooks; early options: %s" %
+               early_opts)
+        for option in self.option_list:
+            option.early_parsing_hook(early_opts)
+
     def add_config_file(self, dest, cfile, reparse=True):
         """ Add a config file, which triggers a full reparse of all
         options. """
@@ -207,10 +256,11 @@ class Parser(argparse.ArgumentParser):
     def reparse(self, argv=None):
         """ Reparse options after they have already been parsed.
 
-        :param argv: The argument list to parse.  By default,
+        :param argv: The argument list to parse. By default,
                      :attr:`Bcfg2.Options.Parser.argv` is reused.
                      (I.e., the argument list that was initially
-                     parsed.)  :type argv: list
+                     parsed.)
+        :type argv: list
         """
         _debug("Reparsing all options")
         self._reset_namespace()
@@ -249,26 +299,7 @@ class Parser(argparse.ArgumentParser):
         # phase 2: re-parse command line for early options; currently,
         # that's database options
         if not self._early:
-            _debug("Option parsing phase 2: Parse early options")
-            early_opts = argparse.Namespace()
-            early_parser = Parser(add_help=False, namespace=early_opts,
-                                  early=True)
-            # add the repo option so we can resolve <repository>
-            # macros
-            early_parser.add_options([repository])
-            early_components = []
-            for component in self.components:
-                if getattr(component, "parse_first", False):
-                    early_components.append(component)
-                    early_parser.add_component(component)
-            early_parser.parse(self.argv)
-            _debug("Early parsing complete, calling hooks")
-            for component in early_components:
-                if hasattr(component, "component_parsed_hook"):
-                    _debug("Calling component_parsed_hook on %s" % component)
-                    getattr(component, "component_parsed_hook")(early_opts)
-            for option in self.option_list:
-                option.early_parsing_hook(early_opts)
+            self._parse_early_options()
         else:
             _debug("Skipping parsing phase 2 in early mode")
 
@@ -299,13 +330,13 @@ class Parser(argparse.ArgumentParser):
         remaining = []
         while not self.parsed:
             self.parsed = True
-            self._set_defaults_from_config()
             _debug("Parsing known arguments")
             try:
                 _, remaining = self.parse_known_args(args=self.argv,
                                                      namespace=self.namespace)
             except OptionParserException:
                 self.error(sys.exc_info()[1])
+            self._set_defaults_from_config()
             self._parse_config_options()
             self._finalize()
         if len(remaining) and not self._early:
diff --git a/testsuite/Testsrc/Testlib/TestOptions/TestComponents.py b/testsuite/Testsrc/Testlib/TestOptions/TestComponents.py
index d637d34c5..61b87de2a 100644
--- a/testsuite/Testsrc/Testlib/TestOptions/TestComponents.py
+++ b/testsuite/Testsrc/Testlib/TestOptions/TestComponents.py
@@ -3,8 +3,8 @@
 import argparse
 import os
 
-from Bcfg2.Options import Option, BooleanOption, ComponentAction, get_parser, \
-    new_parser, Types, ConfigFileAction
+from Bcfg2.Options import Option, BooleanOption, PathOption, ComponentAction, \
+    get_parser, new_parser, Types, ConfigFileAction, Common
 
 from testsuite.Testsrc.Testlib.TestOptions import make_config, One, Two, \
     OptionTestCase
@@ -51,18 +51,27 @@ class ConfigFileComponent(object):
                       default="bar")]
 
 
+class PathComponent(object):
+    """fake component for testing <repository> macros in child components."""
+    options = [PathOption(cf=("test", "test_path")),
+               PathOption(cf=("test", "test_path_default"),
+                          default="<repository>/test/default")]
+
+
 class ParentComponentAction(ComponentAction):
     """parent component loader action."""
     mapping = {"one": ComponentOne,
                "two": ComponentTwo,
                "three": ComponentThree,
-               "config": ConfigFileComponent}
+               "config": ConfigFileComponent,
+               "path": PathComponent}
 
 
 class TestComponentOptions(OptionTestCase):
     """test cases for component loading."""
 
     def setUp(self):
+        OptionTestCase.setUp(self)
         self.options = [
             Option("--parent", type=Types.comma_list,
                    default=["one", "two"], action=ParentComponentAction)]
@@ -147,6 +156,16 @@ class TestComponentOptions(OptionTestCase):
         self.parser.parse(["-C", config_file, "--parent", "config"])
         self.assertEqual(self.result.config2, None)
 
+    @make_config({"test": {"test_path": "<repository>/test"}})
+    def test_macros_in_component_options(self, config_file):
+        """fix up <repository> macros in component options."""
+        self.parser.add_options([Common.repository])
+        self.parser.parse(["-C", config_file, "-Q", "/foo/bar",
+                           "--parent", "path"])
+        self.assertEqual(self.result.test_path, "/foo/bar/test")
+        self.assertEqual(self.result.test_path_default,
+                         "/foo/bar/test/default")
+
 
 class ImportComponentAction(ComponentAction):
     """action that imports real classes for testing."""
diff --git a/testsuite/Testsrc/Testlib/TestOptions/TestOptions.py b/testsuite/Testsrc/Testlib/TestOptions/TestOptions.py
index b76cd6d3a..9f4a7873c 100644
--- a/testsuite/Testsrc/Testlib/TestOptions/TestOptions.py
+++ b/testsuite/Testsrc/Testlib/TestOptions/TestOptions.py
@@ -20,6 +20,7 @@ class TestBasicOptions(OptionTestCase):
         # that's probably bad -- and it's definitely bad if we ever
         # want to do real on-the-fly config changes -- but it's easier
         # to leave it as is and set the options on each test.
+        OptionTestCase.setUp(self)
         self.options = [
             BooleanOption("--test-true-boolean", env="TEST_TRUE_BOOLEAN",
                           cf=("test", "true_boolean"), default=True),
@@ -367,13 +368,16 @@ class TestBasicOptions(OptionTestCase):
             parser.add_options,
             [Option(cf=("test", "option"))])
 
-    @make_config()
+    @make_config({"test": {"test_path": "<repository>/test"}})
     def test_repository_macro(self, config_file):
         """fix up <repository> macros."""
         result = argparse.Namespace()
         parser = Parser(namespace=result)
         parser.add_options([PathOption("--test1"),
                             PathOption("--test2"),
+                            PathOption(cf=("test", "test_path")),
+                            PathOption(cf=("test", "test_path_default"),
+                                       default="<repository>/test/default"),
                             Common.repository])
         parser.parse(["-C", config_file, "-Q", "/foo/bar",
                       "--test1", "<repository>/test1",
@@ -381,6 +385,8 @@ class TestBasicOptions(OptionTestCase):
         self.assertEqual(result.repository, "/foo/bar")
         self.assertEqual(result.test1, "/foo/bar/test1")
         self.assertEqual(result.test2, "/foo/bar/foo/bar")
+        self.assertEqual(result.test_path, "/foo/bar/test")
+        self.assertEqual(result.test_path_default, "/foo/bar/test/default")
 
     @make_config()
     def test_file_like_path_option(self, config_file):
diff --git a/testsuite/Testsrc/Testlib/TestOptions/__init__.py b/testsuite/Testsrc/Testlib/TestOptions/__init__.py
index 688f4e54c..ca2c41359 100644
--- a/testsuite/Testsrc/Testlib/TestOptions/__init__.py
+++ b/testsuite/Testsrc/Testlib/TestOptions/__init__.py
@@ -4,7 +4,7 @@ import os
 import tempfile
 
 from Bcfg2.Compat import wraps, ConfigParser
-from Bcfg2.Options import Parser
+from Bcfg2.Options import Parser, PathOption
 from testsuite.common import Bcfg2TestCase
 
 
@@ -75,7 +75,10 @@ class OptionTestCase(Bcfg2TestCase):
         Parser.unit_test = False
         Bcfg2TestCase.setUpClass()
 
+    def setUp(self):
+        Bcfg2TestCase.setUp(self)
+        PathOption.repository = None
+
     @classmethod
     def tearDownClass(cls):
         Parser.unit_test = True
-        Bcfg2TestCase.tearDownClass()