Launchpad itself

Merge lp:~jml/launchpad/db-revision-bug-487628 into lp:launchpad

  1. db-revision-bug-487628
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Approved by: Henning Eggers
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jml/launchpad/db-revision-bug-487628
Merge into: lp:launchpad
Diff against target: 53 lines (+11/-7)
2 files modified
lib/canonical/database/revision.py (+5/-6)
lib/canonical/testing/layers.py (+6/-1)
To merge this branch: bzr merge lp:~jml/launchpad/db-revision-bug-487628
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Approve
Review via email: mp+15805@code.launchpad.net

Commit message

Raise an informative error when we cannot start the appserver due to unapplied database patches. Make the error itself more useful by recommending running make schema.

To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote :

Hello Reviewer!

This branch fixes a bug and a glitch. It fixes bug 487628, reported by Danilo, where the test suite would raise confusing errors when running appserver tests when the database wasn't properly patched.

The problem is that the AppServer layer starts the app server in its 'setUp' phase. The FunctionLayer only checks the database patch is correct in the 'testSetUp' phase, which is only run after all of the 'setUp' layers are finished.

The solution is to do the same check that the functional layer does in the code that starts up the app server, so the server is only started if the database is correctly patched.

The glitch is that when you actually get the InvalidDatabaseRevision error, it's not clear what you need to do to fix the problem. I've changed the exception text to suggest running make schema.

<adiroiban> after running rocketfuel-get do I need to manualy merge the db changes?
<adiroiban> i got this error canonical.database.revision.InvalidDatabaseRevision: patch-2207-10-0.sql has not been applied to the database
<ajmitch> I think that requires 'make schema', doesn't it?

That's it. I look forward to your review.

jml

Revision history for this message
Henning Eggers (henninge) wrote :

Thank your this fix and making it easier for developers to work on Launchpad.

As discussed on IRC you checked that no doctest depends on the exception text. Thank you for that.

I don't expect a test for testing code, because who is then testing the tester of the tester then?

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/database/revision.py'
--- lib/canonical/database/revision.py 2009-07-30 16:09:14 +0000
+++ lib/canonical/database/revision.py 2009-12-09 05:26:14 +0000
@@ -57,9 +57,9 @@
57 for patch_tuple in fs_patches:57 for patch_tuple in fs_patches:
58 if patch_tuple not in db_patches:58 if patch_tuple not in db_patches:
59 raise InvalidDatabaseRevision(59 raise InvalidDatabaseRevision(
60 "patch-%04d-%02d-%d.sql has not been applied to the database"60 "patch-%04d-%02d-%d.sql has not been applied to the "
61 % patch_tuple61 "database. You probably want to run 'make schema'."
62 )62 % patch_tuple)
6363
64 # Raise an exeption if we have a patch applied to the database that64 # Raise an exeption if we have a patch applied to the database that
65 # cannot be found on the filesystem. We ignore patches with a non-zero65 # cannot be found on the filesystem. We ignore patches with a non-zero
@@ -73,9 +73,8 @@
73 if patch_tuple[2] == 0 and patch_tuple not in fs_patches:73 if patch_tuple[2] == 0 and patch_tuple not in fs_patches:
74 raise InvalidDatabaseRevision(74 raise InvalidDatabaseRevision(
75 "patch-%04d-%02d-%d.sql has been applied to the database "75 "patch-%04d-%02d-%d.sql has been applied to the database "
76 "but does not exist in this source code tree"76 "but does not exist in this source code tree. You probably "
77 % patch_tuple77 "want to run 'make schema'." % patch_tuple)
78 )
7978
8079
81def confirm_dbrevision_on_startup(*ignored):80def confirm_dbrevision_on_startup(*ignored):
8281
=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py 2009-12-08 14:50:07 +0000
+++ lib/canonical/testing/layers.py 2009-12-09 05:26:15 +0000
@@ -88,7 +88,8 @@
8888
89from canonical.lazr import pidfile89from canonical.lazr import pidfile
90from canonical.config import CanonicalConfig, config, dbconfig90from canonical.config import CanonicalConfig, config, dbconfig
91from canonical.database.revision import confirm_dbrevision91from canonical.database.revision import (
92 confirm_dbrevision, confirm_dbrevision_on_startup)
92from canonical.database.sqlbase import cursor, ZopelessTransactionManager93from canonical.database.sqlbase import cursor, ZopelessTransactionManager
93from canonical.launchpad.interfaces import IMailBox, IOpenLaunchBag94from canonical.launchpad.interfaces import IMailBox, IOpenLaunchBag
94from canonical.launchpad.ftests import ANONYMOUS, login, logout, is_logged_in95from canonical.launchpad.ftests import ANONYMOUS, login, logout, is_logged_in
@@ -1564,6 +1565,10 @@
1564 from canonical.launchpad.ftests.harness import LaunchpadTestSetup1565 from canonical.launchpad.ftests.harness import LaunchpadTestSetup
1565 # The database must be available for the app server to start.1566 # The database must be available for the app server to start.
1566 LaunchpadTestSetup().setUp()1567 LaunchpadTestSetup().setUp()
1568 # The app server will not start at all if the database hasn't been
1569 # correctly patched. The app server will make exactly this check,
1570 # doing it here makes the error more obvious.
1571 confirm_dbrevision_on_startup()
1567 _config = cls.appserver_config1572 _config = cls.appserver_config
1568 cmd = [1573 cmd = [
1569 os.path.join(_config.root, 'bin', 'run'),1574 os.path.join(_config.root, 'bin', 'run'),