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
1=== modified file 'lib/canonical/database/revision.py'
2--- lib/canonical/database/revision.py 2009-07-30 16:09:14 +0000
3+++ lib/canonical/database/revision.py 2009-12-09 05:26:14 +0000
4@@ -57,9 +57,9 @@
5 for patch_tuple in fs_patches:
6 if patch_tuple not in db_patches:
7 raise InvalidDatabaseRevision(
8- "patch-%04d-%02d-%d.sql has not been applied to the database"
9- % patch_tuple
10- )
11+ "patch-%04d-%02d-%d.sql has not been applied to the "
12+ "database. You probably want to run 'make schema'."
13+ % patch_tuple)
14
15 # Raise an exeption if we have a patch applied to the database that
16 # cannot be found on the filesystem. We ignore patches with a non-zero
17@@ -73,9 +73,8 @@
18 if patch_tuple[2] == 0 and patch_tuple not in fs_patches:
19 raise InvalidDatabaseRevision(
20 "patch-%04d-%02d-%d.sql has been applied to the database "
21- "but does not exist in this source code tree"
22- % patch_tuple
23- )
24+ "but does not exist in this source code tree. You probably "
25+ "want to run 'make schema'." % patch_tuple)
26
27
28 def confirm_dbrevision_on_startup(*ignored):
29
30=== modified file 'lib/canonical/testing/layers.py'
31--- lib/canonical/testing/layers.py 2009-12-08 14:50:07 +0000
32+++ lib/canonical/testing/layers.py 2009-12-09 05:26:15 +0000
33@@ -88,7 +88,8 @@
34
35 from canonical.lazr import pidfile
36 from canonical.config import CanonicalConfig, config, dbconfig
37-from canonical.database.revision import confirm_dbrevision
38+from canonical.database.revision import (
39+ confirm_dbrevision, confirm_dbrevision_on_startup)
40 from canonical.database.sqlbase import cursor, ZopelessTransactionManager
41 from canonical.launchpad.interfaces import IMailBox, IOpenLaunchBag
42 from canonical.launchpad.ftests import ANONYMOUS, login, logout, is_logged_in
43@@ -1564,6 +1565,10 @@
44 from canonical.launchpad.ftests.harness import LaunchpadTestSetup
45 # The database must be available for the app server to start.
46 LaunchpadTestSetup().setUp()
47+ # The app server will not start at all if the database hasn't been
48+ # correctly patched. The app server will make exactly this check,
49+ # doing it here makes the error more obvious.
50+ confirm_dbrevision_on_startup()
51 _config = cls.appserver_config
52 cmd = [
53 os.path.join(_config.root, 'bin', 'run'),