launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04858
[Merge] lp:~stub/launchpad/dboptions into lp:launchpad
Stuart Bishop has proposed merging lp:~stub/launchpad/dboptions into lp:launchpad with lp:~stub/launchpad/trivial as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~stub/launchpad/dboptions/+merge/73824
= Summary =
Add --port command line support to scripts using db_options(), allowing us to connect to non-standard database ports such as the one pgbouncer is running on.
== Proposed fix ==
== Pre-implementation notes ==
== Implementation details ==
This also tidies up some antique code. There is still a lot of cruft in there, but it is better now.
== Tests ==
== Demo and Q/A ==
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/canonical/launchpad/scripts/tests/test_rundoctests.py
lib/canonical/lp/__init__.py
lib/canonical/database/sqlbase.py
lib/canonical/launchpad/scripts/__init__.py
--
https://code.launchpad.net/~stub/launchpad/dboptions/+merge/73824
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stub/launchpad/dboptions into lp:launchpad.
=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py 2011-07-20 09:59:47 +0000
+++ lib/canonical/database/sqlbase.py 2011-09-02 14:12:28 +0000
@@ -35,7 +35,6 @@
from datetime import datetime
-import re
from textwrap import dedent
import warnings
@@ -297,32 +296,26 @@
# This is only used by scripts, so we must connect to the read-write
# DB here -- that's why we use rw_main_master directly.
- main_connection_string = dbconfig.rw_main_master
+ from canonical.database.postgresql import ConnectionString
+ main_connection_string = ConnectionString(dbconfig.rw_main_master)
# Override dbname and dbhost in the connection string if they
# have been passed in.
- if dbname is not None:
- main_connection_string = re.sub(
- r'dbname=\S*', r'dbname=%s' % dbname, main_connection_string)
- else:
- match = re.search(r'dbname=(\S*)', main_connection_string)
- if match is not None:
- dbname = match.group(1)
-
- if dbhost is not None:
- main_connection_string = re.sub(
- r'host=\S*', r'host=%s' % dbhost, main_connection_string)
- else:
- match = re.search(r'host=(\S*)', main_connection_string)
- if match is not None:
- dbhost = match.group(1)
- return main_connection_string, dbname, dbhost
+ if dbname is None:
+ dbname = main_connection_string.dbname
+ else:
+ main_connection_string.dbname = dbname
+
+ if dbhost is None:
+ dbhost = main_connection_string.host
+ else:
+ main_connection_string.host = dbhost
+
+ return str(main_connection_string), dbname, dbhost
@classmethod
def initZopeless(cls, dbname=None, dbhost=None, dbuser=None,
isolation=ISOLATION_LEVEL_DEFAULT):
- # Connect to the auth master store as well, as some scripts might need
- # to create EmailAddresses and Accounts.
main_connection_string, dbname, dbhost = (
cls._get_zopeless_connection_config(dbname, dbhost))
@@ -348,12 +341,9 @@
})
if dbuser:
- # XXX 2009-05-07 stub bug=373252: Scripts should not be connecting
- # as the launchpad_auth database user.
overlay += dedent("""\
[launchpad]
dbuser: %(dbuser)s
- auth_dbuser: launchpad_auth
""" % {'dbuser': dbuser})
if cls._installed is not None:
@@ -622,7 +612,7 @@
...
TypeError: Use either positional or keyword values with sqlvalue.
- """ # ' <- fix syntax highlighting
+ """
if (values and kwvalues) or (not values and not kwvalues):
raise TypeError(
"Use either positional or keyword values with sqlvalue.")
@@ -652,7 +642,7 @@
return '"%s"' % identifier.replace('"', '""')
-quoteIdentifier = quote_identifier # Backwards compatibility for now.
+quoteIdentifier = quote_identifier # Backwards compatibility for now.
def convert_storm_clause_to_string(storm_clause):
@@ -813,28 +803,24 @@
programs like pg_dump or embed in slonik scripts.
"""
from canonical import lp
- # We start with the config string from the config file, and overwrite
- # with the passed in dbname or modifications made by db_options()
- # command line arguments. This will do until db_options gets an overhaul.
- con_str_overrides = []
+
# We must connect to the read-write DB here, so we use rw_main_master
# directly.
- con_str = dbconfig.rw_main_master
- assert 'user=' not in con_str, (
- 'Connection string already contains username')
+ from canonical.database.postgresql import ConnectionString
+ con_str = ConnectionString(dbconfig.rw_main_master)
+ assert con_str.user is None, (
+ 'Connection string already contains username')
if user is not None:
- con_str_overrides.append('user=%s' % user)
+ con_str.user = user
if lp.dbhost is not None:
- con_str = re.sub(r'host=\S*', '', con_str) # Remove stanza if exists.
- con_str_overrides.append('host=%s' % lp.dbhost)
+ con_str.host = lp.dbhost
+ if lp.dbport is not None:
+ con_str.port = lp.dbport
if dbname is None:
- dbname = lp.get_dbname() # Note that lp.dbname may be None.
+ dbname = lp.get_dbname() # Note that lp.dbname may be None
if dbname is not None:
- con_str = re.sub(r'dbname=\S*', '', con_str) # Remove if exists.
- con_str_overrides.append('dbname=%s' % dbname)
-
- con_str = ' '.join([con_str] + con_str_overrides)
- return con_str
+ con_str.dbname = dbname
+ return str(con_str)
class cursor:
=== modified file 'lib/canonical/launchpad/scripts/__init__.py'
--- lib/canonical/launchpad/scripts/__init__.py 2011-07-26 12:35:52 +0000
+++ lib/canonical/launchpad/scripts/__init__.py 2011-09-02 14:12:28 +0000
@@ -19,7 +19,6 @@
import atexit
import os
import sys
-from textwrap import dedent
import threading
from zope.configuration.config import ConfigurationMachine
@@ -37,6 +36,7 @@
logger,
logger_options,
)
+from canonical.database.postgresql import ConnectionString
# Intentional re-export, following along the lines of the logger module.
from canonical.launchpad.scripts.loghandlers import WatchedFileHandler
from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
@@ -118,7 +118,7 @@
def db_options(parser):
"""Add and handle default database connection options on the command line
- Adds -d (--database), -H (--host) and -U (--user)
+ Adds -d (--database), -H (--host), -P (--port) and -U (--user)
Parsed options provide dbname, dbhost and dbuser attributes.
@@ -134,20 +134,26 @@
To test, we first need to store the current values so we can reset them
later.
- >>> dbname, dbhost, dbuser = lp.get_dbname(), lp.dbhost, lp.dbuser
+ >>> dbname, dbhost, dbport, dbuser = (
+ ... lp.get_dbname(), lp.dbhost, lp.dbport, lp.dbuser)
Ensure that command line options propagate to where we say they do
+ >>> from optparse import OptionParser
>>> parser = OptionParser()
>>> db_options(parser)
>>> options, args = parser.parse_args(
- ... ['--dbname=foo', '--host=bar', '--user=baz'])
- >>> options.dbname, lp.get_dbname(), config.database.dbname
- ('foo', 'foo', 'foo')
- >>> (options.dbhost, lp.dbhost, config.database.dbhost)
- ('bar', 'bar', 'bar')
+ ... ['--dbname=foo', '--host=bar', '--user=baz', '--port=6432'])
+ >>> options.dbname, lp.get_dbname()
+ ('foo', 'foo')
+ >>> (options.dbhost, lp.dbhost)
+ ('bar', 'bar')
>>> (options.dbuser, lp.dbuser)
('baz', 'baz')
+ >>> (options.dbport, lp.dbport)
+ (6432, 6432)
+ >>> config.database.rw_main_master
+ 'dbname=foo user=baz host=bar port=6432'
Make sure that the default user is None
@@ -159,15 +165,29 @@
Reset config
- >>> lp.dbhost, lp.dbuser = dbhost, dbuser
+ >>> lp.dbhost, lp.dbport, lp.dbuser = dbhost, dbport, dbuser
"""
+ startup_connection_string = ConnectionString(
+ config.database.rw_main_master)
+
+ def update_db_config(**kw):
+ connection_string_keys = [
+ 'rw_main_master',
+ 'rw_main_slave',
+ 'ro_main_master',
+ 'ro_main_slave',
+ ]
+ config_data = ["[database]"]
+ for con_str_key in connection_string_keys:
+ con_str = ConnectionString(getattr(config.database, con_str_key))
+ for kwarg, kwval in kw.items():
+ setattr(con_str, kwarg, kwval)
+ config_data.append("%s: %s" % (con_str_key, str(con_str)))
+ config.push('update_db_config', '\n'.join(config_data))
+
def dbname_callback(option, opt_str, value, parser):
parser.values.dbname = value
- config_data = dedent("""
- [database]
- dbname: %s
- """ % value)
- config.push('dbname_callback', config_data)
+ update_db_config(dbname=value)
lp.dbname_override = value
parser.add_option(
@@ -179,11 +199,7 @@
def dbhost_callback(options, opt_str, value, parser):
parser.values.dbhost = value
- config_data = dedent("""
- [database]
- dbhost: %s
- """ % value)
- config.push('dbhost_callback', config_data)
+ update_db_config(host=value)
lp.dbhost = value
parser.add_option(
@@ -192,8 +208,21 @@
help="Hostname or IP address of PostgreSQL server."
)
+ def dbport_callback(options, opt_str, value, parser):
+ value = int(value)
+ parser.values.dbport = value
+ update_db_config(port=value)
+ lp.dbport = value
+
+ parser.add_option(
+ "-p", "--port", action="callback", callback=dbport_callback,
+ type=int, dest="dbport", default=lp.dbport,
+ help="Port PostgreSQL server is listening on."
+ )
+
def dbuser_callback(options, opt_str, value, parser):
parser.values.dbuser = value
+ update_db_config(user=value)
lp.dbuser = value
parser.add_option(
@@ -206,6 +235,6 @@
# as a PostgreSQL user named the same as the current Unix user').
# If the -U option was not given on the command line, our callback is
# never called so we need to set this different default here.
- # Same for dbhost
lp.dbuser = None
- lp.dbhost = None
+ lp.dbhost = startup_connection_string.host
+ lp.dbport = startup_connection_string.port
=== modified file 'lib/canonical/launchpad/scripts/tests/test_rundoctests.py'
--- lib/canonical/launchpad/scripts/tests/test_rundoctests.py 2010-10-12 01:11:41 +0000
+++ lib/canonical/launchpad/scripts/tests/test_rundoctests.py 2011-09-02 14:12:28 +0000
@@ -12,10 +12,12 @@
def tearDown(test):
reset_logging()
+
def test_suite():
suite = unittest.TestSuite()
suite.addTest(DocTestSuite('canonical.launchpad.scripts.sort_sql'))
suite.addTest(DocTestSuite(
'canonical.launchpad.scripts.logger', tearDown=tearDown
))
+ suite.addTest(DocTestSuite('canonical.launchpad.scripts'))
return suite
=== modified file 'lib/canonical/lp/__init__.py'
--- lib/canonical/lp/__init__.py 2011-02-21 00:08:07 +0000
+++ lib/canonical/lp/__init__.py 2011-09-02 14:12:28 +0000
@@ -12,8 +12,8 @@
__metaclass__ = type
import os
-import re
+from canonical.database.postgresql import ConnectionString
from canonical.config import dbconfig
from canonical.database.sqlbase import (
ISOLATION_LEVEL_DEFAULT, ZopelessTransactionManager)
@@ -36,6 +36,7 @@
# office environment.
dbhost = os.environ.get('LP_DBHOST', None)
dbuser = os.environ.get('LP_DBUSER', None)
+dbport = os.environ.get('LP_DBPORT', None)
dbname_override = os.environ.get('LP_DBNAME', None)
@@ -46,15 +47,15 @@
"""
if dbname_override is not None:
return dbname_override
- match = re.search(r'dbname=(\S*)', dbconfig.main_master)
- assert match is not None, 'Invalid main_master connection string'
- return match.group(1)
+ dbname = ConnectionString(dbconfig.main_master).dbname
+ assert dbname is not None, 'Invalid main_master connection string'
+ return dbname
if dbhost is None:
- match = re.search(r'host=(\S*)', dbconfig.main_master)
- if match is not None:
- dbhost = match.group(1)
+ dbhost = ConnectionString(dbconfig.main_master).host
+if dbport is None:
+ dbport = ConnectionString(dbconfig.main_master).port
if dbuser is None:
dbuser = dbconfig.dbuser
@@ -80,7 +81,7 @@
# "Passing dbuser parameter to initZopeless will soon "
# "be mandatory", DeprecationWarning, stacklevel=2
# )
- pass # Disabled. Bug#3050
+ pass # Disabled. Bug #3050
if dbname is None:
dbname = get_dbname()
if dbhost is None:
