Merge lp:~vila/bzr/907279-override-from-env into lp:bzr

[Vincent Ladeuil]

Configuration option value can be overridden by os environ variables

This provides Option.override_from_env as a sibling of default_from_env but
specifiying environ variables that overrides all configuration options
definitions.

I.e. it matches several use cases, the later encountered being BZR_EMAIL
which I used as an example here.

I was strongly tempted to add an 'env_encoding' parameter to complete the
set but would need some feedback from mgz about the functions to be used in
osutils to get there.

[Jelmer Vernooij (jelmer)]

197 + def setUp(self):
198 + super(EmailOptionTests, self).setUp()
This seems unnecessary ?

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

There are two whitespaces around 169/170 in the patch, there should just be one.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Martin Packman (gz)]

The use of osutils.get_user_encoding is completely fine here, will be the locale set on nix, and the same codepage on windows as was used to encode unicode value to bytes by the OS. The main things to worry about are the error handling (on nix it may throw a UnicodeEncodeError as it's possible to have arbitrary bytes in the environment), and could use the new win32utils function to get a unicode environment variable. Perhaps what's needed is something that will do both, decode the value on nix and give a pretty error, and get unicode straight on windows. I was avoiding this as it should be possible to handle many values (and eventually paths) without decoding from bytes on nix, but perhaps it's the right level for the current status of bzrlib.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-12-27 12:18:36 +0000
+++ bzrlib/config.py	2012-01-03 09:54:28 +0000
@@ -1652,17 +1652,6 @@
     raise errors.NoWhoami()
 
 
-def email_from_store(unicode_str):
-    """Unlike other env vars, BZR_EMAIL takes precedence over config settings.
-
-    Whatever comes from a config file is then overridden.
-    """
-    value = os.environ.get('BZR_EMAIL')
-    if value:
-        return value.decode(osutils.get_user_encoding())
-    return unicode_str
-
-
 def _auto_user_id():
     """Calculate automatic user identification.
 
@@ -2343,13 +2332,16 @@
     encoutered, in which config files it can be stored.
     """
 
-    def __init__(self, name, default=None, default_from_env=None,
-                 help=None, from_unicode=None, invalid=None,
-                 unquote=True):
+    def __init__(self, name, override_from_env=None,
+                 default=None, default_from_env=None,
+                 help=None, from_unicode=None, invalid=None, unquote=True):
         """Build an option definition.
 
         :param name: the name used to refer to the option.
 
+        :param override_from_env: A list of environment variables which can
+           provide override any configuration setting.
+
         :param default: the default value to use when none exist in the config
             stores. This is either a string that ``from_unicode`` will convert
             into the proper type, a callable returning a unicode string so that
@@ -2379,9 +2371,12 @@
            safely unquote them (see http://pad.lv/906897). It is provided so
            daughter classes can handle the quoting themselves.
         """
+        if override_from_env is None:
+            override_from_env = []
         if default_from_env is None:
             default_from_env = []
         self.name = name
+        self.override_from_env = override_from_env
         # Convert the default value to a unicode string so all values are
         # strings internally before conversion (via from_unicode) is attempted.
         if default is None:
@@ -2429,6 +2424,17 @@
                 raise errors.ConfigOptionValueError(self.name, unicode_value)
         return converted
 
+    def get_override(self):
+        value = None
+        for var in self.override_from_env:
+            try:
+                # If the env variable is defined, its value takes precedence
+                value = os.environ[var].decode(osutils.get_user_encoding())
+                break
+            except KeyError:
+                continue
+        return value
+
     def get_default(self):
         value = None
         for var in self.default_from_env:
@@ -2703,8 +2709,7 @@
     Option('editor',
            help='The command called to launch an editor to enter a message.'))
 option_registry.register(
-    Option('email', default=default_email,
-           from_unicode=email_from_store,
+    Option('email', override_from_env=['BZR_EMAIL'], default=default_email,
            help='The users identity'))
 option_registry.register(
     Option('gpg_signing_command',
@@ -3468,19 +3473,7 @@
         if expand is None:
             expand = _get_expand_default_value()
         value = None
-        # Ensuring lazy loading is achieved by delaying section matching (which
-        # implies querying the persistent storage) until it can't be avoided
-        # anymore by using callables to describe (possibly empty) section
-        # lists.
         found_store = None # Where the option value has been found
-        for sections in self.sections_def:
-            for store, section in sections():
-                value = section.get(name)
-                if value is not None:
-                    found_store = store
-                    break
-            if value is not None:
-                break
         # If the option is registered, it may provide additional info about
         # value handling
         try:
@@ -3488,9 +3481,10 @@
         except KeyError:
             # Not registered
             opt = None
+
         def expand_and_convert(val):
-            # This may need to be called twice if the value is None or ends up
-            # being None during expansion or conversion.
+            # This may need to be called in different contexts if the value is
+            # None or ends up being None during expansion or conversion.
             if val is not None:
                 if expand:
                     if isinstance(val, basestring):
@@ -3504,11 +3498,30 @@
                 else:
                     val = opt.convert_from_unicode(found_store, val)
             return val
-        value = expand_and_convert(value)
-        if opt is not None and value is None:
-            # If the option is registered, it may provide a default value
-            value = opt.get_default()
-            value = expand_and_convert(value)
+
+        # First of all, check if the environment can override the configuration
+        # value
+        if opt is not None and opt.override_from_env:
+            value = opt.get_override()
+            value = expand_and_convert(value)
+        if value is None:
+            # Ensuring lazy loading is achieved by delaying section matching
+            # (which implies querying the persistent storage) until it can't be
+            # avoided anymore by using callables to describe (possibly empty)
+            # section lists.
+            for sections in self.sections_def:
+                for store, section in sections():
+                    value = section.get(name)
+                    if value is not None:
+                        found_store = store
+                        break
+                if value is not None:
+                    break
+            value = expand_and_convert(value)
+            if opt is not None and value is None:
+                # If the option is registered, it may provide a default value
+                value = opt.get_default()
+                value = expand_and_convert(value)
         for hook in ConfigHooks['get']:
             hook(self, name, value)
         return value

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-12-21 20:32:50 +0000
+++ bzrlib/tests/test_config.py	2012-01-03 09:54:28 +0000
@@ -3377,6 +3377,35 @@
         self.assertRaises(TypeError, conf_stack.get, 'foo')
 
 
+class TestStackWithSimpleStore(tests.TestCase):
+
+    def setUp(self):
+        super(TestStackWithSimpleStore, self).setUp()
+        self.overrideAttr(config, 'option_registry', config.OptionRegistry())
+        self.registry = config.option_registry
+
+    def get_conf(self, content=None):
+        return config.MemoryStack(content)
+
+    def test_override_value_from_env(self):
+        self.registry.register(
+            config.Option('foo', default='bar', override_from_env=['FOO']))
+        self.overrideEnv('FOO', 'quux')
+        # Env variable provides a default taking over the option one
+        conf = self.get_conf('foo=store')
+        self.assertEquals('quux', conf.get('foo'))
+
+    def test_first_override_value_from_env_wins(self):
+        self.registry.register(
+            config.Option('foo', default='bar',
+                          override_from_env=['NO_VALUE', 'FOO', 'BAZ']))
+        self.overrideEnv('FOO', 'foo')
+        self.overrideEnv('BAZ', 'baz')
+        # The first env var set wins
+        conf = self.get_conf('foo=store')
+        self.assertEquals('foo', conf.get('foo'))
+
+
 class TestMemoryStack(tests.TestCase):
 
     def test_get(self):
@@ -4589,21 +4618,22 @@
 class EmailOptionTests(tests.TestCase):
 
     def test_default_email_uses_BZR_EMAIL(self):
+        conf = config.MemoryStack('email=[e-mail]')
         # BZR_EMAIL takes precedence over EMAIL
         self.overrideEnv('BZR_EMAIL', '[e-mail]')
         self.overrideEnv('EMAIL', '[e-mail]')
-        self.assertEquals('[e-mail]', config.default_email())
+        self.assertEquals('[e-mail]', conf.get('email'))
 
     def test_default_email_uses_EMAIL(self):
+        conf = config.MemoryStack('')
         self.overrideEnv('BZR_EMAIL', None)
         self.overrideEnv('EMAIL', '[e-mail]')
-        self.assertEquals('[e-mail]', config.default_email())
+        self.assertEquals('[e-mail]', conf.get('email'))
 
     def test_BZR_EMAIL_overrides(self):
+        conf = config.MemoryStack('email=[e-mail]')
         self.overrideEnv('BZR_EMAIL', '[e-mail]')
-        self.assertEquals('[e-mail]',
-            config.email_from_store('[e-mail]'))
+        self.assertEquals('[e-mail]', conf.get('email'))
         self.overrideEnv('BZR_EMAIL', None)
         self.overrideEnv('EMAIL', '[e-mail]')
-        self.assertEquals('[e-mail]',
-            config.email_from_store('[e-mail]'))
+        self.assertEquals('[e-mail]', conf.get('email'))

=== modified file 'doc/developers/configuration.txt'
--- doc/developers/configuration.txt	2011-12-21 20:32:50 +0000
+++ doc/developers/configuration.txt	2012-01-03 09:54:28 +0000
@@ -186,6 +186,10 @@
   suitable value for the option. If the string cannot be coerced it should
   return None.
 
+* override_from_env: a list of environment variables. The first variable set
+  will be used as the option value overriding any other definition of the
+  option.
+
 * default: the default value that Stack.get() should return if no value can
   be found for the option. This can also be a callable as long as it returns
   a unicode string.

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-22 18:52:58 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-01-03 09:54:28 +0000
@@ -28,6 +28,10 @@
   parent directories.
   (Jared Hance, Jelmer Vernooij, #253529)
 
+* ``config.Option`` can now declare ``override_from_env``, a list of
+  environment variables which, when set, that takes precedence over values
+  defined in configuration files. (Vincent Ladeuil, #907279)
+
 Improvements
 ************