launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/overridable-dbconfig into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/overridable-dbconfig/+merge/76316

Allow DatabaseConfig's settings to be overridden, so production code doesn't have to push ugly config overlay stacks and handle crufty database config sections. initZopeless will be rebuilt on top of this.
-- 
https://code.launchpad.net/~wgrant/launchpad/overridable-dbconfig/+merge/76316
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/overridable-dbconfig into lp:launchpad.

=== modified file 'daemons/librarian.tac'
--- daemons/librarian.tac	2011-03-11 13:37:12 +0000
+++ daemons/librarian.tac	2011-09-21 01:36:18 +0000
@@ -25,7 +25,9 @@
 from lp.services.twistedsupport.loggingsupport import set_up_oops_reporting
 
 # Connect to database
-dbconfig.setConfigSection('librarian')
+dbconfig.override(
+    dbuser=config.librarian.dbuser,
+    isolation_level=config.librarian.isolation_level)
 execute_zcml_for_scripts()
 
 if os.environ.get('LP_TEST_INSTANCE'):

=== modified file 'lib/canonical/config/__init__.py'
--- lib/canonical/config/__init__.py	2011-09-16 09:31:34 +0000
+++ lib/canonical/config/__init__.py	2011-09-21 01:36:18 +0000
@@ -402,8 +402,11 @@
         raise ValueError(
                 "Invalid log level %s. "
                 "Should be DEBUG, CRITICAL, ERROR, FATAL, INFO, WARNING "
-                "as per logging module." % value
-                )
+                "as per logging module." % value)
+
+
+class DatabaseConfigOverrides(object):
+    pass
 
 
 class DatabaseConfig:
@@ -420,6 +423,9 @@
         'dbuser', 'rw_main_master', 'rw_main_slave', 'ro_main_master',
         'ro_main_slave'])
 
+    def __init__(self):
+        self.reset()
+
     @property
     def main_master(self):
         # Its a bit silly having ro_main_master and rw_main_master.
@@ -438,6 +444,23 @@
         else:
             return self.rw_main_slave
 
+    def override(self, **kwargs):
+        """Override one or more config attributes.
+
+        Overriding a value to None removes the override.
+        """
+        for attr, value in kwargs.iteritems():
+            assert attr in self._db_config_attrs, (
+                "%s cannot be overriden" % attr)
+            if value is None:
+                if hasattr(self.overrides, attr):
+                    delattr(self.overrides, attr)
+            else:
+                setattr(self.overrides, attr, value)
+
+    def reset(self):
+        self.overrides = DatabaseConfigOverrides()
+
     def setConfigSection(self, section_name):
         self._config_section = section_name
 
@@ -456,7 +479,7 @@
         overlay = config
         for part in self._config_section.split('.'):
             overlay = getattr(overlay, part)
-        return [overlay, config.database]
+        return [self.overrides, overlay, config.database]
 
     def __getattr__(self, name):
         sections = self._getConfigSections()

=== modified file 'lib/canonical/config/tests/test_database_config.py'
--- lib/canonical/config/tests/test_database_config.py	2011-09-05 15:42:27 +0000
+++ lib/canonical/config/tests/test_database_config.py	2011-09-21 01:36:18 +0000
@@ -3,7 +3,11 @@
 
 __metaclass__ = type
 
-from canonical.config import config, dbconfig
+from canonical.config import (
+    config,
+    DatabaseConfig,
+    dbconfig,
+    )
 from canonical.launchpad.readonly import read_only_file_exists
 from canonical.launchpad.tests.readonly import (
     remove_read_only_file,
@@ -37,6 +41,39 @@
         self.assertEquals(expected_db, dbconfig.rw_main_master)
         self.assertEquals('launchpad_main', dbconfig.dbuser)
 
+    def test_override(self):
+        # dbuser and isolation_level can be overridden at runtime, without
+        # requiring a custom config overlay.
+        dbc = DatabaseConfig()
+        dbc.setConfigSection('launchpad')
+        self.assertEqual('launchpad_main', dbc.dbuser)
+        self.assertEqual('serializable', dbc.isolation_level)
+
+        # dbuser and isolation_level overrides both work.
+        dbc.override(dbuser='not_launchpad', isolation_level='autocommit')
+        self.assertEqual('not_launchpad', dbc.dbuser)
+        self.assertEqual('autocommit', dbc.isolation_level)
+
+        # Overriding dbuser again preserves the isolation_level override.
+        dbc.override(dbuser='also_not_launchpad')
+        self.assertEqual('also_not_launchpad', dbc.dbuser)
+        self.assertEqual('autocommit', dbc.isolation_level)
+
+        # Overriding with None removes the override.
+        dbc.override(dbuser=None, isolation_level=None)
+        self.assertEqual('launchpad_main', dbc.dbuser)
+        self.assertEqual('serializable', dbc.isolation_level)
+
+    def test_reset(self):
+        # reset() removes any overrides.
+        dbc = DatabaseConfig()
+        dbc.setConfigSection('launchpad')
+        self.assertEqual('launchpad_main', dbc.dbuser)
+        dbc.override(dbuser='not_launchpad')
+        self.assertEqual('not_launchpad', dbc.dbuser)
+        dbc.reset()
+        self.assertEqual('launchpad_main', dbc.dbuser)
+
     def test_required_values(self):
         # Some variables are required to have a value, such as dbuser.  So we
         # get a ValueError if they are not set.

=== modified file 'lib/canonical/launchpad/webapp/ftests/test_adapter.txt'
--- lib/canonical/launchpad/webapp/ftests/test_adapter.txt	2011-09-19 13:25:14 +0000
+++ lib/canonical/launchpad/webapp/ftests/test_adapter.txt	2011-09-21 01:36:18 +0000
@@ -423,10 +423,8 @@
     >>> print store.execute("select current_user").get_one()[0]
     launchpad_main
 
-    >>> from canonical.config import config, dbconfig
-    >>> config.statistician.dbuser
-    'statistician'
-    >>> dbconfig.setConfigSection('statistician')
+    >>> from canonical.config import dbconfig
+    >>> dbconfig.override(dbuser='statistician')
     >>> reset_store()
     >>> print store.execute("select current_user").get_one()[0]
     statistician
@@ -452,9 +450,9 @@
     <BLANKLINE>
     >>> transaction.abort()
 
-So you need to explicitly set the user back:
+So you need to explicitly set the user back to the default:
 
-    >>> dbconfig.setConfigSection('launchpad')
+    >>> dbconfig.override(dbuser=None)
     >>> reset_store()
     >>> print store.execute("select current_user").get_one()[0]
     launchpad_main

=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py	2011-09-18 13:04:13 +0000
+++ lib/canonical/testing/layers.py	2011-09-21 01:36:18 +0000
@@ -217,7 +217,10 @@
     sure the right data is available.
     """
     disconnect_stores()
-    dbconfig.setConfigSection(database_config_section)
+    section = getattr(config, database_config_section)
+    dbconfig.override(
+        dbuser=getattr(section, 'dbuser', None),
+        isolation_level=getattr(section, 'isolation_level', None))
 
     main_store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
     assert main_store is not None, 'Failed to reconnect'