Merge lp:~vila/awsome/config-with-tests into lp:awsome

[Vincent Ladeuil]

This add a config implementation with tests simplifying the handling of all
config options:

- each option is declared only once,

- a single section is used in the config file (see below for rationale),

- command line switches are recognized for all options,

- options can get their default value from the environment variables.

We don't need multiple sections for the few options we handle, but since
ConfigParser requires one, I used 'awsome'.

Using a single section also allows to mirror all options as command line
switches and remove the need to always specify a section name when querying
the config.

I used bzr config as a guide to get a simplified design and most of the
tests.

This is a pre-requisite to another proposal I'll file later.

The idea is that we need a simple way to setup tests and environment
variables are not a good solution for that. That's not mentioning that some
of them are currently acquired once (users.py for example) and cannot easily
be changed for a test.

[Martin Packman (gz)]

+Default values can be acquired from environment variables for backwards
+compatibility during a transition period.

So, I disagree somewhat here as we've discussed before. For development, I like not having to copy secret values into a config file to test against different clouds, and I don't see how that changes.

+ if not isinstance(value, str):
+ raise AssertionError(
+ 'Callable default values should be strings')

Assertions without any context at all are a bit annoying.

+class MultilineVersion(argparse.Action):

Slightly confusing copying this in in this mp as it's only removed from bin/awsome in the followup, but not harmful.

+def bool_from_string(string):
+ val = None
+ if isinstance(string, str):
+ try:
+ val = _valid_boolean_strings[string.lower()]
+ except KeyError:
+ pass
+ return val

I'd use a more compact spelling avoiding the isinstance check:

    try:
        return _valid_boolean_strings[string.lower()]
    except (AttributeError, KeyError):
        return None

Or `str.lower(string)` catching TypeError if you really want to be picky about the type.

+def set_env_variable(name, value):
...
+def overrideEnv(test, name, value):
...
+def overrideAttr(test, obj, attr_name, new=_unitialized_attr):

I'd stick all these in awsome.tests so we have a one place with all the extra testing bits we needed to bring in.

+ def test_first_default_value_from_env_wins(self):
+ opt = config.Option('foo', default='bar',
+ default_from_env=['NO_VALUE', 'FOO', 'BAZ'])

Not isolated from NO_VALUE being set in the test process.

+ self.assertNotEquals(None, option_help)
+ # Come on, think about the user, he really wants to know what the
+ # option is about
+ self.assertIsNot(None, option_help)

First assertion is meaningless given the second, nothing ordinary compares equal to None except itself.

+ def test_error(self):

Not wild about this test, overriding standard streams and catching SystemExit is a little dodgy in tests. Would prefer some refactoring to avoid that, but I suspect argparse doesn't make our lives easy here so it's not that important.

review: Approve

[Vincent Ladeuil (vila)]

>>>>> Martin Packman writes:

    > Review: Approve
    > +Default values can be acquired from environment variables for backwards
    > +compatibility during a transition period.

    > So, I disagree somewhat here as we've discussed before. For
    > development, I like not having to copy secret values into a config
    > file to test against different clouds, and I don't see how that
    > changes.

Ha, I failed to fully explain what I have in mind here, sorry.

So, the idea is that you setup a test cloud as part of your dev env and,
separately, fire the test suite.

The test suite checks the availability of various test clouds and inject
the corresponding scenarios to exercise them.

Roughly you do (imaginary commands):

  start-cloud essex-precise
  make test
  hack
  make test
  stop-cloud essex-precise
  hack, edit test cloud config file
  start-cloud essex-precise
  make test

Obviously if you have more than one test cloud, relying on env vars is a
no-go.

And if/when we have such test clouds, manual testing should not be
necessary anymore (or become so awkward we abandon it ;).

Additionally, running bin/awsome without any prior sourcing:

vila:~/src/awsome/trunk :) $ bin/awsome
Traceback (most recent call last):
  File "bin/awsome", line 43, in <module>
    credentials = Credentials.from_config(conf)
  File "/home/vila/src/awsome/trunk/awsome/users.py", line 58, in from_config
    self = cls(conf.get('aws_access_key'), conf.get('aws_secret_key'))
  File "/home/vila/src/awsome/trunk/awsome/users.py", line 25, in __init__
    AWSCredentials.__init__(self, access_key, secret_key)
  File "/home/vila/lib/python/txaws/credentials.py", line 34, in __init__
    raise ValueError("Could not find %s" % ENV_ACCESS_KEY)
ValueError: Could not find AWS_ACCESS_KEY_ID

This demonstrates (don't let the 'from_config' distract you, this is an
old bug also encountered before this mp) that all of us have been
running our manual tests in sourced environments without realizing
*this* issue (there may be others).

'all' here includes me even if --os_auth_scheme=passthrough doesn't
require to set any access key... and is a valid setup for awsome on a
server.

The trap is easy to fall into :-/

Far too easy.

The comment is a reminder of the above issues/missing features, we can
continue to discuss ;)

    > + if not isinstance(value, str):
    > + raise AssertionError(
    > + 'Callable default values should be strings')

    > Assertions without any context at all are a bit annoying.

I've added the option name.

    > +class MultilineVersion(argparse.Action):

    > Slightly confusing copying this in in this mp as it's only removed
    > from bin/awsome in the followup, but not harmful.

Yeah, that was the easiest way to separate the mps.

    > +def bool_from_string(string):
    > + val = None
    > + if isinstance(string, str):
    > + try:
    > + val = _valid_boolean_strings[string.lower()]
    > + except KeyError:
    > + pass
    > + return val

    > I'd use a more compact spelling avoiding the isinstance check:

    > try:
    > return _va...

=== added file 'awsome/config.py'
--- awsome/config.py	1970-01-01 00:00:00 +0000
+++ awsome/config.py	2012-05-15 11:25:23 +0000
@@ -0,0 +1,272 @@
+# Copyright (C) 2012 Canonical Ltd.
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, version 3.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Awsome configuration management.
+
+awsome can be configured:
+- from the command line,
+- via a config file.
+
+Default values can be acquired from environment variables for backwards
+compatibility during a transition period.
+"""
+
+import argparse
+import collections
+import ConfigParser
+import os
+
+import awsome
+
+class ConfigOptionValueError(StandardError):
+
+    _fmt = ('Bad value "%(value)s" for option "%(name)s".')
+
+    def __init__(self, name, value):
+        StandardError.__init__(self, self._fmt % dict(name=name, value=value))
+
+
+class Option(object):
+    """An option definition.
+
+    The option values comes from a config file or the command line.
+    """
+    def __init__(self, name, default=None,  default_from_env=None,
+                 help=None, convert=None, invalid=None):
+        """Build an option definition.
+
+        :param name: the name used to refer to the option.
+
+        :param default: the default value to use when none exist in the config
+            file. This is either a string that ``convert`` will convert into
+            the proper type, a callable returning a string so that ``convert``
+            can be used on the return value, or a python object that can be
+            stringified.
+
+        :param default_from_env: A list of environment variables which can
+           provide a default value. 'default' will be used only if none of the
+           variables specified here are set in the environment.
+
+        :param help: a doc string to explain the option to the user.
+
+        :param convert: a callable to convert the string representing the
+            option value in a config file.
+
+        :param invalid: the action to be taken when an invalid value is
+            encountered in a config file. This is called only when ``convert``
+            is invoked to convert a string and returns None or raise ValueError
+            or TypeError. Accepted values are: None (ignore invalid values),
+            'error' (raises ConfigOptionValueError exception).
+        """
+        self.name = name
+        # Convert the default value to a string so all values are strings
+        # internally before conversion (via from_unicode) is attempted.
+        if default is None:
+            self.default = None
+        elif isinstance(default, (str, bool, int)):
+            # Rely on python to convert the supported types to strings
+            self.default = '%s' % (default,)
+        elif callable(default):
+            self.default = default
+        else:
+            # other python objects are not expected
+            raise AssertionError('%r is not supported as a default value'
+                                 % (default,))
+        if default_from_env is None:
+            default_from_env = []
+        self.default_from_env = default_from_env
+        self.help = help
+        self.convert = convert
+        if invalid and invalid not in ('error',):
+            raise AssertionError("%s not supported for 'invalid'" % (invalid,))
+        self.invalid = invalid
+
+    def get_default(self):
+        value = None
+        for var in self.default_from_env:
+            try:
+                # If the env variable is defined, its value is the default one
+                value = os.environ[var]
+                break
+            except KeyError:
+                continue
+        if value is None:
+            # Otherwise, fallback to the value defined at registration
+            if callable(self.default):
+                value = self.default()
+                if not isinstance(value, str):
+                    raise AssertionError(
+                    'Callable default values should be strings')
+            else:
+                value = self.default
+        return value
+
+    def convert_from_string(self, string):
+        if self.convert is None or string is None:
+            # Don't convert or nothing to convert
+            return string
+        try:
+            converted = self.convert(string)
+        except (ValueError, TypeError):
+            # Invalid values are ignored
+            converted = None
+        if converted is None and self.invalid is not None:
+            # The conversion failed
+            if self.invalid == 'error':
+                raise ConfigOptionValueError(self.name, string)
+        return converted
+
+
+class MultilineVersion(argparse.Action):
+    """Alternative to argparse 'version' action supporting multiline output"""
+
+    def __init__(self, option_strings, message_format,
+            dest=argparse.SUPPRESS, default=argparse.SUPPRESS,
+            help="display version and exit", **kwargs):
+        super(MultilineVersion, self).__init__(option_strings=option_strings,
+            nargs=0, dest=dest, default=default, help=help)
+        self.fmt = message_format
+        self.details = kwargs
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        parser.exit(message=self.fmt % dict(prog=parser.prog, **self.details))
+
+
+class Config(ConfigParser.ConfigParser):
+    """A container for all awsome configuration options.
+
+    This provides a single place to query for any option and can be populated
+    from the command line (providing switches for all options, see
+    ``from_cmdline``).
+
+    Registering options is mandatory.
+    """
+
+    # ConfigParser mandates a section so we use 'awsome' just in case we
+    # 'DEFAULT' for other purposes in the future.
+    # This allows removing the section parameter from .get() and .set()
+    awsome_section_name = 'awsome'
+
+    def __init__(self):
+        ConfigParser.ConfigParser.__init__(self)
+        self.add_section(self.awsome_section_name)
+
+    def get(self, name):
+        value = None
+        opt = option_registry.get(name)
+        try:
+            value = ConfigParser.ConfigParser.get(
+                self, self.awsome_section_name, name)
+        except ConfigParser.NoOptionError:
+            value = None
+        if value is None and opt is not None:
+            # Get the default value
+            value = opt.get_default()
+        if opt is not None:
+            # Convert the value
+            value = opt.convert_from_string(value)
+        return value
+
+    def set(self, name, value):
+        return ConfigParser.ConfigParser.set(self, self.awsome_section_name,
+                                             name, value)
+
+    def from_cmdline(self, args):
+        """Populate option values from a command line.
+
+        :param args: A list of strings as expected in sys.argv[1:].
+        """
+        parser = argparse.ArgumentParser(description="AWS API Proxy",
+                                         prog='awsome')
+        parser.add_argument("--version", action=MultilineVersion,
+                            message_format="%(prog)s %(version)s\n%(copy)s\n",
+                            version=awsome.__version__,
+                            copy="(C) 2012 Canonical Ltd.")
+        # Add all registered config options as cmdline arguments
+        for name, opt in option_registry.iteritems():
+            arg_name = '--%s' % (name.replace('_', '-'),)
+            parser.add_argument(arg_name, type=str, help=opt.help)
+        args = parser.parse_args(args)
+        # Inject the parsed arguments is they are not None
+        for k,v in vars(args).items():
+            if v is not None:
+                # Option values are always strings
+                self.set(k, v)
+
+
+# Predefined converters to get proper values from strings
+
+_valid_boolean_strings = dict(yes=True, no=False,
+                              y=True, n=False,
+                              on=True, off=False,
+                              true=True, false=False)
+_valid_boolean_strings['1'] = True
+_valid_boolean_strings['0'] = False
+
+
+def bool_from_string(string):
+    val = None
+    if isinstance(string, str):
+        try:
+            val = _valid_boolean_strings[string.lower()]
+        except KeyError:
+            pass
+    return val
+
+
+def int_from_string(string):
+    return int(string)
+
+
+class OptionRegistry(object):
+    """Register config options by their name."""
+
+    def __init__(self):
+        # We use an OrderedDict so that the registration order is preserved
+        # giving a way to display the options in the logical order used to
+        # group them.
+        self._dict = collections.OrderedDict()
+
+    def register(self, option):
+        if option.name in self._dict:
+            raise KeyError('Key %r already registered' % key)
+        self._dict[option.name] = option
+
+    def get(self, key):
+        return self._dict[key]
+
+    def remove(self, key):
+        del self._dict[key]
+
+    def iteritems(self):
+        return self._dict.iteritems()
+
+
+option_registry = OptionRegistry()
+
+# Not strictly speaking a config option as it makes no sense to set the config
+# file *inside* a config file but this helps support the command line use case
+# and provide a default value
+option_registry.register(
+    Option('config', default='/etc/awsome.conf',
+           help='''Configuration file path.'''))
+
+# Registered options (the order used below the one that wil be displayed by
+# argparse when displaying usage.
+
+option_registry.register(
+    Option('port', default='8080', convert=int_from_string,
+           help='''Local port to listen on.'''))
+option_registry.register(
+    Option('log_file', default='/var/log/awsome.log',
+           help='''Path to log file.'''))

=== modified file 'awsome/tests/__init__.py'
--- awsome/tests/__init__.py	2012-05-01 09:48:32 +0000
+++ awsome/tests/__init__.py	2012-05-15 11:25:23 +0000
@@ -84,6 +84,7 @@
     names = [
         'awsome',
         'blackbox',
+        'config',
         'handlers',
         'proxy_aws',
         'proxy_openstack',

=== added file 'awsome/tests/test_config.py'
--- awsome/tests/test_config.py	1970-01-01 00:00:00 +0000
+++ awsome/tests/test_config.py	2012-05-15 11:25:23 +0000
@@ -0,0 +1,265 @@
+# Copyright (C) 2012 Canonical Ltd.
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, version 3.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from cStringIO import StringIO
+import os
+import sys
+import unittest
+
+from awsome import (
+    config,
+    tests,
+    )
+
+
+# To correctly apply scenarios with most runners when running module directly,
+# need both load_tests (for unittest/testtools) and test_suite (for twisted)
+load_tests = tests.load_tests
+test_suite = tests.get_suite_from_name(__name__)
+
+
+def set_env_variable(name, value):
+    """Modify the environment, setting or removing the env_variable.
+
+    :param name: The environment variable to set.
+
+    :param value: The value to set the environment to. If None, then
+        the variable will be removed.
+
+    :return: The original value of the environment variable.
+    """
+    orig_val = os.environ.get(name)
+    if value is None:
+        if orig_val is not None:
+            # The variable should be deleted
+            del os.environ[name]
+    else:
+        os.environ[name] = value
+    return orig_val
+
+def overrideEnv(test, name, value):
+    """Set an environment variable, and reset it after the test.
+
+    :param test: The test case.
+
+    :param name: The environment variable name.
+
+    :param new: The value to set the variable to. If None, the 
+        variable is deleted from the environment.
+
+    :returns: The actual variable value.
+    """
+    orig = set_env_variable(name, value)
+    test.addCleanup(set_env_variable, name, orig)
+    return orig
+
+
+_unitialized_attr = object()
+"""A sentinel needed to act as a default value in a method signature."""
+
+
+def overrideAttr(test, obj, attr_name, new=_unitialized_attr):
+    """Overrides an object attribute restoring it after the test.
+
+    :param test: The test case.
+
+    :param obj: The object that will be mutated.
+
+    :param attr_name: The attribute name we want to preserve/override in
+        the object.
+
+    :param new: The optional value we want to set the attribute to.
+
+    :returns: The actual attr value.
+    """
+    value = getattr(obj, attr_name)
+    # The actual value is captured by the call below
+    test.addCleanup(setattr, obj, attr_name, value)
+    if new is not _unitialized_attr:
+        setattr(obj, attr_name, new)
+    return value
+
+
+class OptionTests(unittest.TestCase):
+
+    def test_default_value(self):
+        opt = config.Option('foo', default='bar')
+        self.assertEquals('bar', opt.get_default())
+
+    def test_callable_default_value(self):
+        def bar_as_str():
+            return 'bar'
+        opt = config.Option('foo', default=bar_as_str)
+        self.assertEquals('bar', opt.get_default())
+
+    def test_default_value_from_env(self):
+        opt = config.Option('foo', default='bar', default_from_env=['FOO'])
+        overrideEnv(self, 'FOO', 'quux')
+        # Env variable provides a default taking over the option one
+        self.assertEquals('quux', opt.get_default())
+
+    def test_first_default_value_from_env_wins(self):
+        opt = config.Option('foo', default='bar',
+                            default_from_env=['NO_VALUE', 'FOO', 'BAZ'])
+        overrideEnv(self, 'FOO', 'foo')
+        overrideEnv(self, 'BAZ', 'baz')
+        # The first env var set wins
+        self.assertEquals('foo', opt.get_default())
+
+    def test_not_supported_object_default_value(self):
+        self.assertRaises(AssertionError, config.Option, 'foo',
+                          default=object())
+
+
+class OptionConverterMixin(object):
+
+    def assertConverted(self, expected, opt, value):
+        self.assertEquals(expected, opt.convert_from_string(value))
+
+    def assertErrors(self, opt, value):
+        self.assertRaises(config.ConfigOptionValueError,
+                          opt.convert_from_string, value)
+
+    def assertConvertInvalid(self, opt, invalid_value):
+        opt.invalid = None
+        self.assertEquals(None, opt.convert_from_string(invalid_value))
+        opt.invalid = 'error'
+        self.assertErrors(opt, invalid_value)
+
+
+class OptionWithBooleanConverterTests(unittest.TestCase, OptionConverterMixin):
+
+    def get_option(self):
+        return config.Option('foo', help='A boolean.',
+                             convert=config.bool_from_string)
+
+    def test_convert_invalid(self):
+        opt = self.get_option()
+        # A string that is not recognized as a boolean
+        self.assertConvertInvalid(opt, 'invalid-boolean')
+        # A list of strings is never recognized as a boolean
+        self.assertConvertInvalid(opt, ['not', 'a', 'boolean'])
+
+    def test_convert_valid(self):
+        opt = self.get_option()
+        self.assertConverted(True, opt, 'True')
+        self.assertConverted(True, opt, '1')
+        self.assertConverted(False, opt, 'False')
+
+
+class OptionWithIntegerConverterTests(unittest.TestCase, OptionConverterMixin):
+
+    def get_option(self):
+        return config.Option('foo', help='An integer.',
+                             convert=config.int_from_string)
+
+    def test_convert_invalid(self):
+        opt = self.get_option()
+        # A string that is not recognized as an integer
+        self.assertConvertInvalid(opt, 'forty-two')
+        # A list of strings is never recognized as an integer
+        self.assertConvertInvalid(opt, ['a', 'list'])
+
+    def test_convert_valid(self):
+        opt = self.get_option()
+        self.assertConverted(16, opt, '16')
+
+
+class OptionRegistryTests(unittest.TestCase):
+
+    def setUp(self):
+        super(OptionRegistryTests, self).setUp()
+        # Always start with an empty registry
+        overrideAttr(self, config, 'option_registry', config.OptionRegistry())
+        self.registry = config.option_registry
+
+    def test_register(self):
+        opt = config.Option('foo')
+        self.registry.register(opt)
+        self.assertIs(opt, self.registry.get('foo'))
+
+    def test_registered_help(self):
+        opt = config.Option('foo', help='A simple option')
+        self.registry.register(opt)
+        self.assertEquals('A simple option', self.registry.get('foo').help)
+
+
+class RegisteredOptionsTests(unittest.TestCase):
+    """All registered options should verify some constraints."""
+
+    scenarios = [(key, {'option_name': key, 'option': option}) for key, option
+                 in config.option_registry.iteritems()]
+
+    def setUp(self):
+        super(RegisteredOptionsTests, self).setUp()
+        self.registry = config.option_registry
+
+    def test_proper_name(self):
+        # An option should be registered under its own name, this can't be
+        # checked at registration time for the lazy ones.
+        self.assertEquals(self.option_name, self.option.name)
+
+    def test_help_is_set(self):
+        option_help = self.registry.get(self.option_name).help
+        self.assertNotEquals(None, option_help)
+        # Come on, think about the user, he really wants to know what the
+        # option is about
+        self.assertIsNot(None, option_help)
+        self.assertNotEquals('', option_help)
+
+
+class ConfigTests(unittest.TestCase):
+
+    def setUp(self):
+        self.conf = config.Config()
+
+    def test_get_unregistered_option(self):
+        self.assertRaises(KeyError, self.conf.get, 'foo')
+
+    def test_get_on_empty_config_gives_default_value(self):
+        # The default value is provided as a string at registration but .get()
+        # convert it
+        self.assertEquals(8080, self.conf.get('port'))
+
+    def test_roundtrip(self):
+        # This allows covering .set()
+        self.conf.set('config', 'foo')
+        self.assertEquals('foo', self.conf.get('config'))
+
+
+class ConfigFromCmdlineTests(unittest.TestCase):
+
+    def setUp(self):
+        self.conf = config.Config()
+
+    def test_default_conf_file(self):
+        self.conf.from_cmdline([])
+        self.assertEquals('/etc/awsome.conf', self.conf.get('config'))
+
+    def test_port(self):
+        self.conf.from_cmdline(['--port=80'])
+        self.assertEquals(80, self.conf.get('port'))
+
+    def assertEndsWith(self, s, suffix):
+        """Asserts that s ends with suffix."""
+        if not s.endswith(suffix):
+            raise AssertionError('string %r does not end with %r' % (s, suffix))
+
+    def test_error(self):
+        err = StringIO()
+        overrideAttr(self, sys, 'stderr', err)
+        with self.assertRaises(SystemExit) as cm:
+            self.conf.from_cmdline(['foo', 'bar'])
+        lines = err.getvalue().splitlines()
+        self.assertEndsWith(lines[-1], 'error: unrecognized arguments: foo bar')
+

=== modified file 'bin/awsome'
--- bin/awsome	2012-05-08 09:04:13 +0000
+++ bin/awsome	2012-05-15 11:25:23 +0000
@@ -16,6 +16,9 @@
 
 import os
 import sys
+
+
+# Find the awsome lib along the script itself
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
 import ConfigParser