launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/dbconfig-section-doom/+merge/76922

DatabaseConfig's section selection support is no longer used. Remove it and a few related methods.

It's currently hardcoded to the 'launchpad' section, as it was the default and is where appserver dbuser configs live. Eventually that should be migrated across and removed.
-- 
https://code.launchpad.net/~wgrant/launchpad/dbconfig-section-doom/+merge/76922
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/dbconfig-section-doom into lp:launchpad.

=== modified file 'lib/canonical/config/__init__.py'
--- lib/canonical/config/__init__.py	2011-09-20 12:18:39 +0000
+++ lib/canonical/config/__init__.py	2011-09-26 00:28:24 +0000
@@ -31,7 +31,6 @@
 
 
 __all__ = [
-    'DatabaseConfig',
     'dbconfig',
     'config',
     ]
@@ -461,25 +460,15 @@
     def reset(self):
         self.overrides = DatabaseConfigOverrides()
 
-    def setConfigSection(self, section_name):
-        self._config_section = section_name
-
-    def getSectionName(self):
-        """The name of the config file section this DatabaseConfig references.
-        """
-        return self._config_section
-
     def _getConfigSections(self):
         """Returns a list of sections to search for database configuration.
 
         The first section in the list has highest priority.
         """
-        if self._config_section is None:
-            return [config.database]
-        overlay = config
-        for part in self._config_section.split('.'):
-            overlay = getattr(overlay, part)
-        return [self.overrides, overlay, config.database]
+        # config.launchpad remains here for compatibility -- production
+        # appserver configs customise its dbuser. Eventually they should
+        # be migrated into config.database, and this can be removed.
+        return [self.overrides, config.launchpad, config.database]
 
     def __getattr__(self, name):
         sections = self._getConfigSections()
@@ -497,4 +486,3 @@
 
 
 dbconfig = DatabaseConfig()
-dbconfig.setConfigSection('launchpad')

=== modified file 'lib/canonical/config/tests/test_database_config.py'
--- lib/canonical/config/tests/test_database_config.py	2011-09-20 12:18:39 +0000
+++ lib/canonical/config/tests/test_database_config.py	2011-09-26 00:28:24 +0000
@@ -3,11 +3,7 @@
 
 __metaclass__ = type
 
-from canonical.config import (
-    config,
-    DatabaseConfig,
-    dbconfig,
-    )
+from canonical.config import DatabaseConfig
 from canonical.launchpad.readonly import read_only_file_exists
 from canonical.launchpad.tests.readonly import (
     remove_read_only_file,
@@ -21,31 +17,9 @@
 
     layer = DatabaseLayer
 
-    def test_overlay(self):
-        # The dbconfig option overlays the database configurations of a
-        # chosen config section over the base section.
-        self.assertRaises(
-            AttributeError, getattr, config.database, 'dbuser')
-        self.assertRaises(
-            AttributeError, getattr, config.launchpad, 'main_master')
-        self.assertEquals('launchpad_main', config.launchpad.dbuser)
-        self.assertEquals('librarian', config.librarian.dbuser)
-
-        dbconfig.setConfigSection('librarian')
-        expected_db = (
-            'dbname=%s host=localhost' % DatabaseLayer._db_fixture.dbname)
-        self.assertEquals(expected_db, dbconfig.rw_main_master)
-        self.assertEquals('librarian', dbconfig.dbuser)
-
-        dbconfig.setConfigSection('launchpad')
-        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.
+        # dbuser and isolation_level can be overridden at runtime.
         dbc = DatabaseConfig()
-        dbc.setConfigSection('launchpad')
         self.assertEqual('launchpad_main', dbc.dbuser)
         self.assertEqual('serializable', dbc.isolation_level)
 
@@ -67,37 +41,28 @@
     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.
-        self.assertRaises(
-            AttributeError, getattr, config.codehosting, 'dbuser')
-        dbconfig.setConfigSection('codehosting')
-        self.assertRaises(ValueError, getattr, dbconfig, 'dbuser')
-        dbconfig.setConfigSection('launchpad')
-
     def test_main_master_and_main_slave(self):
         # DatabaseConfig provides two extra properties: main_master and
         # main_slave, which return the value of either
         # rw_main_master/rw_main_slave or ro_main_master/ro_main_slave,
         # depending on whether or not we're in read-only mode.
+        dbc = DatabaseConfig()
         self.assertFalse(read_only_file_exists())
-        self.assertEquals(dbconfig.rw_main_master, dbconfig.main_master)
-        self.assertEquals(dbconfig.rw_main_slave, dbconfig.main_slave)
+        self.assertEquals(dbc.rw_main_master, dbc.main_master)
+        self.assertEquals(dbc.rw_main_slave, dbc.main_slave)
 
         touch_read_only_file()
         try:
             self.assertTrue(read_only_file_exists())
             self.assertEquals(
-                dbconfig.ro_main_master, dbconfig.main_master)
+                dbc.ro_main_master, dbc.main_master)
             self.assertEquals(
-                dbconfig.ro_main_slave, dbconfig.main_slave)
+                dbc.ro_main_slave, dbc.main_slave)
         finally:
             remove_read_only_file()

=== modified file 'lib/canonical/testing/ftests/test_layers.py'
--- lib/canonical/testing/ftests/test_layers.py	2011-09-20 22:33:07 +0000
+++ lib/canonical/testing/ftests/test_layers.py	2011-09-26 00:28:24 +0000
@@ -31,10 +31,7 @@
     getUtility,
     )
 
-from canonical.config import (
-    config,
-    dbconfig,
-    )
+from canonical.config import config
 from canonical.lazr.pidfile import pidfile_path
 from canonical.librarian.client import (
     LibrarianClient,
@@ -480,20 +477,6 @@
     want_memcached = True
     want_rabbitmq = True
 
-    def testSwitchDbConfig(self):
-        # Test that we can switch database configurations, and that we
-        # end up connected as the right user.
-
-        self.assertEqual(dbconfig.dbuser, 'launchpad_main')
-        LaunchpadScriptLayer.switchDbConfig('librarian')
-        self.assertEqual(dbconfig.dbuser, 'librarian')
-
-        from canonical.database.sqlbase import cursor
-        cur = cursor()
-        cur.execute('SELECT current_user;')
-        user = cur.fetchone()[0]
-        self.assertEqual(user, 'librarian')
-
 
 class LayerProcessControllerInvariantsTestCase(BaseTestCase):
     layer = AppServerLayer

=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py	2011-09-22 19:59:40 +0000
+++ lib/canonical/testing/layers.py	2011-09-26 00:28:24 +0000
@@ -1460,11 +1460,6 @@
     def testTearDown(cls):
         disconnect_stores()
 
-    @classmethod
-    @profiled
-    def switchDbConfig(cls, database_config_section):
-        reconnect_stores(database_config_section=database_config_section)
-
 
 class LaunchpadScriptLayer(ZopelessLayer, LaunchpadLayer):
     """Testing layer for scripts using the main Launchpad database adapter"""
@@ -1498,11 +1493,6 @@
     def testTearDown(cls):
         disconnect_stores()
 
-    @classmethod
-    @profiled
-    def switchDbConfig(cls, database_config_section):
-        reconnect_stores(database_config_section=database_config_section)
-
 
 class LaunchpadTestSetup(PgTestSetup):
     template = 'launchpad_ftest_template'

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-09-19 06:57:55 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-09-26 00:28:24 +0000
@@ -35,10 +35,7 @@
     removeSecurityProxy,
     )
 
-from canonical.config import (
-    config,
-    dbconfig,
-    )
+from canonical.config import config
 from canonical.database.constants import UTC_NOW
 from canonical.database.sqlbase import flush_database_caches
 from canonical.launchpad.testing.pages import (
@@ -609,14 +606,13 @@
         # error_description on the given job. Which is a PITA.
         distroseries = job.distroseries
         transaction.commit()
-        starting_database_config_section = dbconfig.getSectionName()
         reconnect_stores("initializedistroseries")
         job = self.job_source.get(distroseries)
         job.start()
         job.fail()
         job.notifyUserError(error)
         transaction.commit()
-        reconnect_stores(starting_database_config_section)
+        reconnect_stores('launchpad')
 
     def test_initialization_failure_explanation_shown(self):
         # When initialization has failed an explanation of the failure can be