launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04882
[Merge] lp:~wgrant/launchpad/destroy-lots-of-db-cruft into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/destroy-lots-of-db-cruft into lp:launchpad with lp:~wgrant/launchpad/deprecated-functions-are-bad as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/destroy-lots-of-db-cruft/+merge/74233
Continuing on from <https://code.launchpad.net/~wgrant/launchpad/deprecated-functions-are-bad/+merge/74225>, this branch removes canonical.lp's internal DB config storage and generally paves the way to storing the DB config in one place: config.database.rw_main_master.
canonical.lp is now left as a pretty pathetic wrapper around ZTM. I haven't yet worked out what I'm going to do about the dbconfig.dbuser default, but it can stay like this for now (ec2 likes it, at least). This does mean that -U and initZopeless will interact badly, but no script seems to use both db_options() and initZopeless(). The next branch in the series cleans that area up and adds assertions to ensure conflicts are detected.
I also deleted some obsolete cruft that would have needed reworking otherwise.
--
https://code.launchpad.net/~wgrant/launchpad/destroy-lots-of-db-cruft/+merge/74233
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/destroy-lots-of-db-cruft into lp:launchpad.
=== removed file 'database/schema/restoredump.sh'
--- database/schema/restoredump.sh 2009-06-24 21:17:33 +0000
+++ database/schema/restoredump.sh 1970-01-01 00:00:00 +0000
@@ -1,35 +0,0 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-#
-# This shell script can be used to restore a production database dump
-
-DMP=launchpad_prod.20050207.pg_dump
-DBNAME=rest
-
-pg_restore -l ${DMP} \
- | grep -v SEQUENCE \
- | grep -v TABLE \
- | grep -v ACL \
- | grep -v INDEX \
- | grep -v CONSTRAINT \
- | grep -v VIEW \
- | grep -v TRIGGER \
- | grep -v COMMENT \
- | grep -v ACL \
- | grep -v BLOBS \
- > r.listing
-pg_restore -l ${DMP} | grep TABLE >> r.listing
-pg_restore -l ${DMP} | grep VIEW >> r.listing
-pg_restore -l ${DMP} | grep INDEX >> r.listing
-pg_restore -l ${DMP} | grep CONSTRAINT >> r.listing
-pg_restore -l ${DMP} | grep TRIGGER >> r.listing
-pg_restore -l ${DMP} | grep SEQUENCE >> r.listing
-pg_restore -l ${DMP} | grep COMMENT >> r.listing
-pg_restore -l ${DMP} | grep BLOBS >> r.listing
-pg_restore -l ${DMP} | grep ACL >> r.listing
-
-
-dropdb ${DBNAME}
-createdb -E UNICODE ${DBNAME}
-pg_restore -U postgres --no-acl --no-owner -L r.listing -d ${DBNAME} -v ${DMP} 2>&1 | grep -v NOTICE
-env LP_DBNAME=${DBNAME} python security.py
=== modified file 'lib/canonical/database/ftests/__init__.py'
--- lib/canonical/database/ftests/__init__.py 2009-06-25 05:39:50 +0000
+++ lib/canonical/database/ftests/__init__.py 2011-09-06 14:40:31 +0000
@@ -1,26 +1,2 @@
# Copyright 2009 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-
-import os
-from canonical.launchpad.daemons.tachandler import TacTestSetup
-
-class PortForwardTestSetup(TacTestSetup):
-
- def setUpRoot(self):
- if os.path.exists(self.pidfile):
- os.remove(self.pidfile)
- if os.path.exists(self.logfile):
- os.remove(self.logfile)
-
- @property
- def tacfile(self):
- return os.path.abspath(os.path.join(os.path.dirname(__file__),
- 'portforward-to-postgres.tac'))
-
- @property
- def logfile(self):
- return os.path.join(os.getcwd(), 'portforward-to-postgres.log')
-
- @property
- def pidfile(self):
- return os.path.join(os.getcwd(), 'portforward-to-postgres.pid')
=== removed file 'lib/canonical/database/ftests/portforward-to-postgres.tac'
--- lib/canonical/database/ftests/portforward-to-postgres.tac 2010-10-20 18:43:29 +0000
+++ lib/canonical/database/ftests/portforward-to-postgres.tac 1970-01-01 00:00:00 +0000
@@ -1,35 +0,0 @@
-# -*- mode: python -*-
-
-# Copyright 2009 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-import os
-
-from twisted.application import service, internet
-from twisted.protocols import portforward
-from twisted.application.internet import TCPServer
-
-import canonical.lp
-from canonical.launchpad.daemons.readyservice import ReadyService
-
-application = service.Application('portforward_to_postgres')
-
-# The libpq library uses the $PGHOST environment variable as the
-# default database host to connect to. Fall back to
-# canonical.lp.dbhost and then localhost.
-if os.environ.get('PGHOST'):
- dbhost = os.environ.get('PGHOST')
-elif canonical.lp.dbhost:
- dbhost = canonical.lp.dbhost
-else:
- dbhost = 'localhost'
-
-port = 5432
-if os.environ.get('PGPORT'):
- port = int(os.environ.get('PGPORT'))
-
-pf = portforward.ProxyFactory(dbhost, port)
-svc = internet.TCPServer(5555, pf)
-svc.setServiceParent(application)
-
-ReadyService().setServiceParent(application)
=== modified file 'lib/canonical/database/harness.py'
--- lib/canonical/database/harness.py 2010-11-23 16:18:39 +0000
+++ lib/canonical/database/harness.py 2011-09-06 14:40:31 +0000
@@ -5,7 +5,7 @@
The scripts provide an interactive prompt with the Launchpad Storm classes,
all interface classes and the zope3 CA-fu at your fingertips, connected to
-launchpad_dev or your LP_DBNAME environment variable (if you have one set).
+launchpad_dev or the database specified on the command line.
One uses Python, the other iPython.
"""
=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py 2011-09-06 14:40:31 +0000
+++ lib/canonical/database/sqlbase.py 2011-09-06 14:40:31 +0000
@@ -800,20 +800,12 @@
Allows you to pass the generated connection details to external
programs like pg_dump or embed in slonik scripts.
"""
- from canonical import lp
-
# We must connect to the read-write DB here, so we use rw_main_master
# directly.
from canonical.database.postgresql import ConnectionString
con_str = ConnectionString(dbconfig.rw_main_master)
if user is not None:
con_str.user = user
- if lp.dbhost is not None:
- 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
if dbname is not None:
con_str.dbname = dbname
return str(con_str)
=== modified file 'lib/canonical/launchpad/scripts/__init__.py'
--- lib/canonical/launchpad/scripts/__init__.py 2011-09-02 14:50:37 +0000
+++ lib/canonical/launchpad/scripts/__init__.py 2011-09-06 14:40:31 +0000
@@ -27,7 +27,6 @@
import zope.sendmail.delivery
import zope.site.hooks
-from canonical import lp
from canonical.config import config
# these are intentional re-exports, apparently, used by *many* files.
from canonical.launchpad.scripts.logger import (
@@ -127,15 +126,8 @@
maintenance tools cannot do this however.
dbname and dbhost are also propagated to config.database.dbname and
- config.database.dbhost. dbname, dbhost and dbuser are also propagated to
- lp.get_dbname(), lp.dbhost and lp.dbuser. This ensures that all systems
- will be using the requested connection details.
-
- To test, we first need to store the current values so we can reset them
- later.
-
- >>> dbname, dbhost, dbport, dbuser = (
- ... lp.get_dbname(), lp.dbhost, lp.dbport, lp.dbuser)
+ config.database.dbhost. This ensures that all systems will be using
+ the requested connection details.
Ensure that command line options propagate to where we say they do
@@ -144,14 +136,14 @@
>>> db_options(parser)
>>> options, args = parser.parse_args(
... ['--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)
+ >>> options.dbname
+ 'foo'
+ >>> options.dbhost
+ 'bar'
+ >>> options.dbuser
+ 'baz'
+ >>> options.dbport
+ 6432
>>> config.database.rw_main_master
'dbname=foo user=baz host=bar port=6432'
>>> config.database.rw_main_slave
@@ -166,15 +158,10 @@
>>> parser = OptionParser()
>>> db_options(parser)
>>> options, args = parser.parse_args([])
- >>> options.dbuser, lp.dbuser
- (None, None)
-
- Reset config
-
- >>> lp.dbhost, lp.dbport, lp.dbuser = dbhost, dbport, dbuser
+ >>> print options.dbuser
+ None
"""
- startup_connection_string = ConnectionString(
- config.database.rw_main_master)
+ conn_string = ConnectionString(config.database.rw_main_master)
def update_db_config(**kw):
connection_string_keys = [
@@ -194,23 +181,20 @@
def dbname_callback(option, opt_str, value, parser):
parser.values.dbname = value
update_db_config(dbname=value)
- lp.dbname_override = value
parser.add_option(
"-d", "--dbname", action="callback", callback=dbname_callback,
- type="string", dest="dbname",
- default=config.database.rw_main_master,
+ type="string", dest="dbname", default=conn_string.dbname,
help="PostgreSQL database to connect to."
)
def dbhost_callback(options, opt_str, value, parser):
parser.values.dbhost = value
update_db_config(host=value)
- lp.dbhost = value
parser.add_option(
"-H", "--host", action="callback", callback=dbhost_callback,
- type="string", dest="dbhost", default=lp.dbhost,
+ type="string", dest="dbhost", default=conn_string.host,
help="Hostname or IP address of PostgreSQL server."
)
@@ -218,29 +202,19 @@
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,
+ type=int, dest="dbport", default=conn_string.port,
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(
"-U", "--user", action="callback", callback=dbuser_callback,
type="string", dest="dbuser", default=None,
help="PostgreSQL user to connect as."
)
-
- # The default user is None for scripts (which translates to 'connect
- # 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.
- lp.dbuser = None
- lp.dbhost = startup_connection_string.host
- lp.dbport = startup_connection_string.port
=== modified file 'lib/canonical/lp/__init__.py'
--- lib/canonical/lp/__init__.py 2011-09-02 14:02:57 +0000
+++ lib/canonical/lp/__init__.py 2011-09-06 14:40:31 +0000
@@ -11,8 +11,6 @@
__metaclass__ = type
-import os
-
from canonical.database.postgresql import ConnectionString
from canonical.config import dbconfig
from canonical.database.sqlbase import (
@@ -20,46 +18,9 @@
__all__ = [
- 'get_dbname', 'dbhost', 'dbuser', 'isZopeless', 'initZopeless',
+ 'isZopeless', 'initZopeless',
]
-# SQLObject compatibility - dbname, dbhost and dbuser are DEPRECATED.
-#
-# Allow override by environment variables for backwards compatibility.
-# This was needed to allow tests to propagate settings to spawned processes.
-# However, now we just have a single environment variable (LAUNCHPAD_CONF)
-# which specifies which section of the config file to use instead,
-# Note that an empty host is different to 'localhost', as the latter
-# connects via TCP/IP instead of a Unix domain socket. Also note that
-# if the host is empty it can be overridden by the standard PostgreSQL
-# environment variables, this feature currently required by Async's
-# 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)
-
-
-def get_dbname():
- """Get the DB Name for scripts: deprecated.
-
- :return: The dbname for scripts.
- """
- if dbname_override is not None:
- return dbname_override
- dbname = ConnectionString(dbconfig.main_master).dbname
- assert dbname is not None, 'Invalid main_master connection string'
- return dbname
-
-
-if dbhost is None:
- dbhost = ConnectionString(dbconfig.main_master).host
-if dbport is None:
- dbport = ConnectionString(dbconfig.main_master).port
-
-if dbuser is None:
- dbuser = dbconfig.dbuser
-
def isZopeless():
"""Returns True if we are running in the Zopeless environment"""
@@ -67,27 +28,12 @@
return ZopelessTransactionManager._installed is not None
-_IGNORED = object()
-
-
def initZopeless(dbname=None, dbhost=None, dbuser=None,
isolation=ISOLATION_LEVEL_DEFAULT):
"""Initialize the Zopeless environment."""
if dbuser is None:
- # Nothing calling initZopeless should be connecting as the
- # 'launchpad' user, which is the default.
- # StuartBishop 20050923
- # warnings.warn(
- # "Passing dbuser parameter to initZopeless will soon "
- # "be mandatory", DeprecationWarning, stacklevel=2
- # )
- pass # Disabled. Bug #3050
- if dbname is None:
- dbname = get_dbname()
- if dbhost is None:
- dbhost = globals()['dbhost']
- if dbuser is None:
- dbuser = globals()['dbuser']
+ dbuser = (
+ ConnectionString(dbconfig.main_master).user or dbconfig.dbuser)
return ZopelessTransactionManager.initZopeless(
dbname=dbname, dbhost=dbhost, dbuser=dbuser, isolation=isolation)
Follow ups
