Merge lp:~jtv/launchpad/bug-536769 into lp:launchpad

[Jeroen T. Vermeulen]

Give buildd manager access to BranchJob.

= Bug 536769 =

The buildd master needs database access to TranslationTemplatesBuildJob in order to execute jobs of that type.

This means giving SELECT and DELETE privileges on BranchJob to fiera.

An update to the TranslationTemplatesBuildBehavior test reproduces the problem. A security.cfg patch fixes it.

To test:
{{{
./bin/test -vv -t test_translationtemplatesbuildbehavior
}}}

To Q/A: go through the instructions at https://dev.launchpad.net/Translations/BuildTemplatesOnLocalBuildFarm without applying the manual patch mentioned for this bug. Watch your postgres logs for a permissions error when fiera accesses branchjob; there should be none.

Note that this is not a schema change as such, and so does not need to go into db-devel.

No lint.

Jeroen

[Jelmer Vernooij (jelmer)]

One minor comment:

19 === modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
20 --- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2010-03-06 04:57:40 +0000
21 +++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2010-03-10 18:05:30 +0000
38 @@ -81,7 +83,12 @@
39 class TestTranslationTemplatesBuildBehavior(TestCaseWithFactory):
40 """Test `TranslationTemplatesBuildBehavior`."""
41
42 - layer = ZopelessDatabaseLayer
43 + layer = LaunchpadZopelessLayer
44 +
45 + def _becomeBuilddMaster(self):
46 + """Log into the database as the buildd master."""
47 + transaction.commit()
^^^ Is the transaction.commit() really necessary here, would a Store.flush not work ?

48 + self.layer.switchDbUser(config.builddmaster.dbuser)
49
50 def _makeBehavior(self):
51 """Create a TranslationTemplatesBuildBehavior.

review: Approve (code mentat)

[Michael Nelson (michael.nelson)]

I've got nothing to add to Jelmer's review.

I did have one question for my own learning though: is this the type of thing you were *against* using the with statement for? ie. creating a context manager that also switched back so that you could do:

with dbUserAs(config.builddmaster.dbuser):
    ...

?

@jelmer: I remembered after our conversation, the review type should be code*. Thanks for the review! I nice easy one to start with :-)

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Hi Jelmer,

Thanks for the review.

> 45 + def _becomeBuilddMaster(self):
> 46 + """Log into the database as the buildd master."""
> 47 + transaction.commit()
> ^^^ Is the transaction.commit() really necessary here, would a Store.flush not
> work ?

Good question to ask! In this case, yes, we're switching database identities. And you can't do that in the same transaction, so flushing wouldn't make the most recent data changes visible.

This is a pattern we come across sometimes in our test: you want to test what you do on the database under realistic permissions, but those permissions aren't enough to set up your test data. The only choice is to commit and switch identities. And not do it too often, of course. :-)

Jeroen

[Jeroen T. Vermeulen (jtv)]

Hi Michael,

> I did have one question for my own learning though: is this the type of thing
> you were *against* using the with statement for? ie. creating a context
> manager that also switched back so that you could do:

No, actually. I do believe Robert noted that that's a bad thing to do when changing directories in production code, but of course that's a far cry from a temporary state in a unit test.

For the record: what I object to in "with" is the exception-handling design of the context-manager protocol. It mixes two completely different goals that I think should not be combined: a "conditional catch/re-raise" that invites clever, obscure, and brittle control flow; and a simple "enter/exit" mechanism that makes "finally" clauses a bit easier to write. (The latter is helpful, though I still have the separate and minor gripe that it puts burdens on the calling code that from a C++ standpoint should be contained in the class that gives rise to them. But "with" did become a real improvement over "finally" with the more recent support for multiple context managers in a single block.)

The justifying use-case for the IMHO dangerous mixing of "conditional catch/re-raise" and "enter/exit" was a transaction handler that would commit implicitly if no exception was raised. Bad idea! That kind of design can work in very limited cases, but it's brittle to the point of irresponsibility. If you want that sort of thing, separate the two: (1) a transaction manager that aborts if not explicitly committed by exit time—note: no need to meddle with exceptions. And (2) a control-flow construct that commits the transaction if the contained block exits normally—note: an explicit commit at the end of the block expresses this about as well, in less code, without breaking up the order of events; and if necessary a conventional function call can give you the same "if we get to the end of all this, commit in a single place" structure.

Once these two things are bound into one context manager, what happens when you discover you need different behavior with particular exception paths? (a) Go back to custom control flow, which is now the suspicious thing to do. (b) Do a little exception dance to present the context manager with the expected situations at block exit. (c) Have more transaction managers (a hierarchy?) for the same resource but with different exception behaviors. All of these are bad.

To me, it would have made more sense to start out with a simpler enter/exit mechanism and see what else is really needed for code instrumentation, customized control flow, or whatever. But that involves weaseling out of fun and challenging design discussions for cool features, and standing up to the snake who says "taste this, it will give you the knowledge of exceptions and God-like power over control flow."

Did someone mention that there's no single right way to do things unless you're Dutch? They say we have opinions on everything. :)

> with dbUserAs(config.builddmaster.dbuser):
> ...
>
> ?

Is there any need to switch back afterwards? I thought this was part of normal test isolation.

Jeroen

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-03-01 21:59:32 +0000
+++ database/schema/security.cfg	2010-03-10 18:05:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 #
 # Possible permissions: SELECT, INSERT, UPDATE, EXECUTE
@@ -838,6 +838,7 @@
 public.archivearch                              = SELECT, UPDATE
 public.archivedependency                        = SELECT
 public.branch                                   = SELECT
+public.branchjob                                = SELECT, DELETE
 public.buildqueue                               = SELECT, INSERT, UPDATE, DELETE
 public.job                                      = SELECT, INSERT, UPDATE, DELETE
 public.buildpackagejob                          = SELECT, INSERT, UPDATE, DELETE

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2010-03-06 04:57:40 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2010-03-10 18:05:30 +0000
@@ -5,12 +5,14 @@
 
 import logging
 from StringIO import StringIO
+import transaction
 from unittest import TestLoader
 
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from canonical.testing import ZopelessDatabaseLayer
+from canonical.config import config
+from canonical.testing import LaunchpadZopelessLayer
 
 from canonical.launchpad.interfaces import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
@@ -81,7 +83,12 @@
 class TestTranslationTemplatesBuildBehavior(TestCaseWithFactory):
     """Test `TranslationTemplatesBuildBehavior`."""
 
-    layer = ZopelessDatabaseLayer
+    layer = LaunchpadZopelessLayer
+
+    def _becomeBuilddMaster(self):
+        """Log into the database as the buildd master."""
+        transaction.commit()
+        self.layer.switchDbUser(config.builddmaster.dbuser)
 
     def _makeBehavior(self):
         """Create a TranslationTemplatesBuildBehavior.
@@ -108,6 +115,7 @@
         behavior._getChroot = FakeChroot
         buildqueue_item = self._getBuildQueueItem(behavior)
 
+        self._becomeBuilddMaster()
         behavior.dispatchBuildToSlave(buildqueue_item, logging)
 
         slave_status = behavior._builder.slaveStatus()