launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/no-config-names-in-store-names into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/no-config-names-in-store-names/+merge/76135

This branch simplifies our Store and DatabaseConfig handling.

DBPolicy and LaunchpadDatabase presently use store names of the form 'configsection-realm-flavor', with LaunchpadDatabase creating its own DatabaseConfig from the given configsection. DBPolicy defaults to using canonical.config.dbconfig's current setting, but provides an override facility -- which nothing uses. So LaunchpadDatabase always ends up being given the current section name. It might as well just retrieve it directly.

So that's what this branch does. Store names are now of the form 'realm-flavor', with LaunchpadDatabase always using the current section set in canonical.config.dbconfig. This paves the way to performing username overrides in dbconfig, rather than using lazr.config overlays.

One would think this might break a lot of stuff, but all broken tests were fixed with a simple s/launchpad-main/main/.

22:40:46 < wgrant> stub: Do you recall why our store names include the config section name? We never change it outside tests...
22:42:17 < wgrant> I guess it may have been a WHUI attempt to permit usage of multiple concurrent configs.
22:43:30  * wgrant deletes it to see what breaks.
22:44:14 < wgrant> Ah, of course. It's the only way to get the section name into LaunchpadDatabase.
22:44:23 < wgrant> Unless I just fix it to always use canonical.config.dbconfig...
22:44:53 < wgrant> Which it already does, because the other case is WHUI.
22:58:45 < stub> wgrant: I suspect the code you are looking at was assembled (I won't say 'designed') before canonical.config.dbconfig existed.

-- 
https://code.launchpad.net/~wgrant/launchpad/no-config-names-in-store-names/+merge/76135
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/no-config-names-in-store-names into lp:launchpad.

=== modified file 'lib/canonical/database/tests/test_zopeless_transaction_manager.py'
--- lib/canonical/database/tests/test_zopeless_transaction_manager.py	2011-09-18 08:28:20 +0000
+++ lib/canonical/database/tests/test_zopeless_transaction_manager.py	2011-09-20 01:44:26 +0000
@@ -23,7 +23,7 @@
     def test_reset_stores_only_does_so_on_active_stores(self):
         active_stores = [item[0] for item in getUtility(IZStorm).iterstores()]
         self.assertContentEqual(
-            ['launchpad-main-master', 'session'], active_stores)
+            ['main-master', 'session'], active_stores)
         ZopelessTransactionManager._reset_stores()
         # If any other stores had been reset, they'd be activated and would
         # then be returned by ZStorm.iterstores().

=== modified file 'lib/canonical/launchpad/database/tests/test_oauth.py'
--- lib/canonical/launchpad/database/tests/test_oauth.py	2011-08-13 08:50:09 +0000
+++ lib/canonical/launchpad/database/tests/test_oauth.py	2011-09-20 01:44:26 +0000
@@ -35,7 +35,7 @@
         """
         zstorm = getUtility(IZStorm)
         self.assertEquals(
-            'launchpad-%s-%s' % (MAIN_STORE, MASTER_FLAVOR),
+            '%s-%s' % (MAIN_STORE, MASTER_FLAVOR),
             zstorm.get_name(self.class_._get_store()))
 
 

=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
--- lib/canonical/launchpad/webapp/adapter.py	2011-09-13 05:23:16 +0000
+++ lib/canonical/launchpad/webapp/adapter.py	2011-09-20 01:44:26 +0000
@@ -53,7 +53,7 @@
 
 from canonical.config import (
     config,
-    DatabaseConfig,
+    dbconfig,
     )
 from canonical.database.interfaces import IRequestExpired
 from canonical.database.postgresql import ConnectionString
@@ -524,38 +524,35 @@
             raise StormAccessFromMainThread()
 
         try:
-            config_section, realm, flavor = self._uri.database.split('-')
+            realm, flavor = self._uri.database.split('-')
         except ValueError:
             raise AssertionError(
-                'Connection uri %s does not match section-realm-flavor format'
+                'Connection uri %s does not match realm-flavor format'
                 % repr(self._uri.database))
 
         assert realm == 'main', 'Unknown realm %s' % realm
         assert flavor in ('master', 'slave'), 'Unknown flavor %s' % flavor
 
-        my_dbconfig = DatabaseConfig()
-        my_dbconfig.setConfigSection(config_section)
-
         # We set self._dsn here rather than in __init__ so when the Store
         # is reconnected it pays attention to any config changes.
         config_entry = '%s_%s' % (realm, flavor)
-        connection_string = getattr(my_dbconfig, config_entry)
+        connection_string = getattr(dbconfig, config_entry)
         assert 'user=' not in connection_string, (
                 "Database username should not be specified in "
                 "connection string (%s)." % connection_string)
 
         # Try to lookup dbuser using the $realm_dbuser key. If this fails,
         # fallback to the dbuser key.
-        dbuser = getattr(my_dbconfig, '%s_dbuser' % realm, my_dbconfig.dbuser)
+        dbuser = getattr(dbconfig, '%s_dbuser' % realm, dbconfig.dbuser)
 
         self._dsn = "%s user=%s" % (connection_string, dbuser)
 
         flags = _get_dirty_commit_flags()
 
-        if my_dbconfig.isolation_level is None:
+        if dbconfig.isolation_level is None:
             self._isolation = ISOLATION_LEVEL_SERIALIZABLE
         else:
-            self._isolation = isolation_level_map[my_dbconfig.isolation_level]
+            self._isolation = isolation_level_map[dbconfig.isolation_level]
 
         raw_connection = super(LaunchpadDatabase, self).raw_connect()
 

=== modified file 'lib/canonical/launchpad/webapp/dbpolicy.py'
--- lib/canonical/launchpad/webapp/dbpolicy.py	2010-12-17 20:36:30 +0000
+++ lib/canonical/launchpad/webapp/dbpolicy.py	2011-09-20 01:44:26 +0000
@@ -88,10 +88,6 @@
     """Base class for database policies."""
     implements(IDatabasePolicy)
 
-    # The section name to retrieve database connection details from.
-    # None means the default.
-    config_section = None
-
     # The default flavor to use.
     default_flavor = MASTER_FLAVOR
 
@@ -103,9 +99,7 @@
         if flavor == DEFAULT_FLAVOR:
             flavor = self.default_flavor
 
-        config_section = self.config_section or dbconfig.getSectionName()
-
-        store_name = '%s-%s-%s' % (config_section, name, flavor)
+        store_name = '%s-%s' % (name, flavor)
         store = getUtility(IZStorm).get(
             store_name, 'launchpad:%s' % store_name)
         if not getattr(store, '_lp_store_initialized', False):

=== modified file 'lib/canonical/launchpad/webapp/ftests/test_adapter.txt'
--- lib/canonical/launchpad/webapp/ftests/test_adapter.txt	2011-02-24 23:53:49 +0000
+++ lib/canonical/launchpad/webapp/ftests/test_adapter.txt	2011-09-20 01:44:26 +0000
@@ -338,7 +338,7 @@
 the database.
 
     >>> get_request_statements()
-    [(0, 0, 'SQL-launchpad-main-master', 'SELECT 2')]
+    [(0, 0, 'SQL-main-master', 'SELECT 2')]
 
 
 When a RequestExpired exception is raised, the current

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2011-08-16 20:38:35 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2011-09-20 01:44:26 +0000
@@ -149,14 +149,14 @@
 
     def test_no_mode_changes(self):
         # Make sure the master/slave stores are present in zstorm.
-        self.assertIn('launchpad-main-master', self.zstorm_stores)
-        self.assertIn('launchpad-main-slave', self.zstorm_stores)
+        self.assertIn('main-master', self.zstorm_stores)
+        self.assertIn('main-slave', self.zstorm_stores)
 
         self.publication.beforeTraversal(self.request)
 
         # Since the mode didn't change, the stores were left in zstorm.
-        self.assertIn('launchpad-main-master', self.zstorm_stores)
-        self.assertIn('launchpad-main-slave', self.zstorm_stores)
+        self.assertIn('main-master', self.zstorm_stores)
+        self.assertIn('main-slave', self.zstorm_stores)
 
         # With the store's connection being the same as before.
         master = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
@@ -171,8 +171,8 @@
 
     def test_changing_modes(self):
         # Make sure the master/slave stores are present in zstorm.
-        self.assertIn('launchpad-main-master', self.zstorm_stores)
-        self.assertIn('launchpad-main-slave', self.zstorm_stores)
+        self.assertIn('main-master', self.zstorm_stores)
+        self.assertIn('main-slave', self.zstorm_stores)
 
         try:
             touch_read_only_file()
@@ -185,8 +185,8 @@
 
         # Here the mode has changed to read-only, so the stores were removed
         # from zstorm.
-        self.assertNotIn('launchpad-main-master', self.zstorm_stores)
-        self.assertNotIn('launchpad-main-slave', self.zstorm_stores)
+        self.assertNotIn('main-master', self.zstorm_stores)
+        self.assertNotIn('main-slave', self.zstorm_stores)
 
         # If they're needed again, they'll be re-created by ZStorm, and when
         # that happens they will point to the read-only databases.