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

[Jeroen T. Vermeulen]

[r=adeuring][bug=733132] DistroSeriesDifferenceJob feature flag.

= Summary =

Add feature flag to control generation of DistroSeriesDifferenceJobs.

Branch builds on the as-yet unlanded one that introduced DistroSeriesDifferenceJob. The flag should control creation of those jobs, and thus the tracking of differences between derived distroseries and their respective parents.

== Proposed fix ==

The hard part is testing database permissions for the scripts that may create the new jobs, when most of the tests will run with job generation disabled.

That's why you'll see a test that assumes each of the known database roles that may want to do this, and at least tries to create a job. That really just tests INSERT privileges, but I'm assuming that it's a reasonable indicator in practice. I don't want the test to get too slow and complicated, especially since the real question is whether I forgot any database roles, not whether I forgot a privilege.

== Pre-implementation notes ==

The naming and setting of the flag gave us some trouble:

 * Its name uses underscores to separate words. An existing related flag used dashes. I informed Julian that the documentation does not allow this, and we'll have to restore consistent naming later.

 * According to William, we now test for boolean flags in python by evaluating them as booleans. This means that the empty string is used to signify "false" and any other string (including "false"!) means "true."

== Implementation details ==

I kept the name of the feature flag in a variable. But it's duplicated in flags.py because nobody else seemed to follow that practice there. I suppose it's convenient to be able to grep for the flag name there, though with the dash/underscore problem it is a tradeoff for convenience over safety.

== Tests ==

{{{
./bin/test -vvc lp.soyuz.tests.test_distroseriesdifferencejob
}}}

== Demo and Q/A ==

It should still be possible to publish source package releases, with or without the new feature flag set.

= Launchpad lint =

The warnings about security.cfg are nonsensical; I wish we could get rid of them somehow. The other ones strike me as dubious as well, so I didn't try to "fix" them. (I didn't cause them either).

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/interfaces/distroseriesdifferencejob.py
  lib/lp/soyuz/model/distroseriesdifferencejob.py
  lib/lp/soyuz/model/distributionjob.py
  lib/lp/registry/tests/test_distroseries.py
  lib/lp/soyuz/model/publishing.py
  database/schema/security.cfg
  lib/lp/registry/interfaces/distroseries.py
  lib/lp/soyuz/configure.zcml
  lib/lp/soyuz/interfaces/distributionjob.py
  lib/lp/soyuz/tests/test_distroseriesdifferencejob.py
  lib/lp/registry/model/distroseries.py
  lib/canonical/launchpad/interfaces/_schema_circular_imports.py

./database/schema/security.cfg
     735: Line exceeds 78 characters.
     736: Line exceeds 78 characters.
     737: Line exceeds 78 characters.
     763: Line exceeds 78 characters.
     767: Line exceeds 78 characters.
     822: Line exceeds 78 characters.
     836: Line exceeds 78 characters.
     837: Line exceeds 78 characters.
     854: Line exceeds 78 characters.
     855: Line exceeds 78 characters.
     856: Line exceeds 78 characters.
     857: Line exceeds 78 characters.
     858: Line exceeds 78 characters.
     911: Line exceeds 78 characters.
     912: Line exceeds 78 characters.
     913: Line exceeds 78 characters.
     943: Line exceeds 78 characters.
    1024: Line exceeds 78 characters.
    1034: Line exceeds 78 characters.
    1035: Line exceeds 78 characters.
./lib/lp/registry/interfaces/distroseries.py
     430: E301 expected 1 blank line, found 2
     471: E301 expected 1 blank line, found 0
./lib/lp/registry/model/distroseries.py
     403: E301 expected 1 blank line, found 2

[Abel Deuring (adeuring)]

(14:12:05) adeuring: jtv: the branch looks good. only one detail: FEATURE_FLAG as a variable name does not tell what the feature is about. Could you rename it to something like ENABLE_DERIVED_SERIES_JOBS ?
(14:12:36) jtv: adeuring: I'm in two minds about that… doesn't the module it's in do enough to disambiguate it?
(14:12:55) jtv: I could import it into the test as distroseriesdifferencejob.FEATURE_FLAG
(14:13:02) adeuring: jtv: well, we could have two feature flags in this module
(14:13:05) jtv: That way the name is always disambiguated.
(14:13:28) jtv: Ah I see what you mean.
(14:14:41) adeuring: I prefer names which tell what is done over how things are done
(14:15:05) jtv: Okay, then how about ENABLING_FEATURE_FLAG?
(14:15:24) jtv: Because it is important to my mind that it is the feature flag, not e.g. a boolean that controls the feature.
(14:15:54) jtv: If the test refers to it as distroseriesdifferencejob.ENABLING_FEATURE_FLAG, I think that'd be about as clear as it could be.
(14:16:01) adeuring: jtv: what about FEATURE_FLAG_ENABLE_DERIVED_JOBS?
(14:16:28) jtv: Gets a bit long, and basically for the purpose of repeating the module name.
(14:16:49) adeuring: without say what is enabled, it looks to me still like a label on an electrial switch just saying "SWITCH" ;)
(14:18:03) jtv: Well that would make sense in the context of a little circuit board when you know what the board is for, just not what outside part should be attached to the particular piece of wire you're looking at.
(14:18:57) adeuring: jtv, ok, so... ENABLE_MODULE? or FLAG_ENALBE_MODULE?
(14:19:18) jtv: It would have to say "flag," but I don't see why it should say "module."
(14:19:58) adeuring: well, I still think that its good to indicate what is enabled
(14:21:31) jtv: Yes, but I'm relying on the "distroseriesdifferencejob" to imply that it's DistroSeriesDifferenceJob that's being enabled.
(14:22:13) jtv: I don't think it'll confuse anyone, especially since it's not even being exported—it's just the test that refers to it from the outside.
(14:23:42) adeuring: jtv: ok, for the test distroseriesdifferencejob.WHAETEVER is more or less fine, but _inside_ the module something like FEATURE_FLAG_ENABLE_MODULE would still make more sense to me
(14:25:47) jtv: OK, I can't say I like it much but you have the advantage of a fresh outside look. I'll use that.

review: Approve (code)

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-02-25 15:53:47 +0000
+++ lib/lp/services/features/flags.py	2011-03-11 13:38:10 +0000
@@ -73,6 +73,10 @@
      'boolean',
      'Enables derivative distributions pages.',
      ''),
+    ('soyuz.derived_series_jobs.enabled',
+     'boolean',
+     "Compute package differences for derived distributions.",
+     ''),
     ('translations.sharing_information.enabled',
      'boolean',
      'Enables display of sharing information on translation pages.',

=== modified file 'lib/lp/soyuz/model/distroseriesdifferencejob.py'
--- lib/lp/soyuz/model/distroseriesdifferencejob.py	2011-03-11 13:38:09 +0000
+++ lib/lp/soyuz/model/distroseriesdifferencejob.py	2011-03-11 13:38:10 +0000
@@ -14,6 +14,7 @@
     )
 
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from lp.services.features import getFeatureFlag
 from lp.services.job.model.job import Job
 from lp.soyuz.interfaces.distributionjob import (
     DistributionJobType,
@@ -28,6 +29,9 @@
     )
 
 
+FEATURE_FLAG_ENABLE_MODULE = u"soyuz.derived_series_jobs.enabled"
+
+
 def make_metadata(sourcepackagename):
     """Return JSON metadata for a job on `sourcepackagename`."""
     return {'sourcepackagename': sourcepackagename.id}
@@ -102,6 +106,8 @@
     @classmethod
     def createForPackagePublication(cls, distroseries, sourcepackagename):
         """See `IDistroSeriesDifferenceJobSource`."""
+        if not getFeatureFlag(FEATURE_FLAG_ENABLE_MODULE):
+            return
         children = distroseries.getDerivedSeries()
         parent = distroseries.parent_series
         for relative in list(children) + [parent]:

=== modified file 'lib/lp/soyuz/tests/test_distroseriesdifferencejob.py'
--- lib/lp/soyuz/tests/test_distroseriesdifferencejob.py	2011-03-11 13:38:09 +0000
+++ lib/lp/soyuz/tests/test_distroseriesdifferencejob.py	2011-03-11 13:38:10 +0000
@@ -5,16 +5,23 @@
 
 __metaclass__ = type
 
+import transaction
+from psycopg2 import ProgrammingError
 from zope.component import getUtility
 from zope.interface.verify import verifyObject
 
-from canonical.testing.layers import ZopelessDatabaseLayer
+from canonical.testing.layers import (
+    LaunchpadZopelessLayer,
+    ZopelessDatabaseLayer,
+    )
+from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.soyuz.interfaces.distroseriesdifferencejob import (
     IDistroSeriesDifferenceJobSource,
     )
 from lp.soyuz.model.distroseriesdifferencejob import (
     create_job,
+    FEATURE_FLAG_ENABLE_MODULE,
     find_waiting_jobs,
     make_metadata,
     may_require_job,
@@ -27,6 +34,10 @@
 
     layer = ZopelessDatabaseLayer
 
+    def setUp(self):
+        super(TestDistroSeriesDifferenceJobSource, self).setUp()
+        self.useFixture(FeatureFixture({FEATURE_FLAG_ENABLE_MODULE: u'on'}))
+
     def getJobSource(self):
         return getUtility(IDistroSeriesDifferenceJobSource)
 
@@ -144,3 +155,41 @@
         jobs = list(find_waiting_jobs(derived_series, package))
         self.assertEqual(1, len(jobs))
         self.assertEqual(package.id, jobs[0].metadata['sourcepackagename'])
+
+    def test_createForPackagePublication_obeys_feature_flag(self):
+        distroseries = self.makeDerivedDistroSeries()
+        package = self.factory.makeSourcePackageName()
+        self.useFixture(FeatureFixture({FEATURE_FLAG_ENABLE_MODULE: ''}))
+        self.getJobSource().createForPackagePublication(distroseries, package)
+        self.assertContentEqual([], find_waiting_jobs(distroseries, package))
+
+
+class TestDistroSeriesDifferenceJobPermissions(TestCaseWithFactory):
+    """Database permissions test for `DistroSeriesDifferenceJob`."""
+
+    layer = LaunchpadZopelessLayer
+
+    def test_permissions(self):
+        script_users = [
+            'archivepublisher',
+            'queued',
+            'uploader',
+            ]
+        derived_series = self.factory.makeDistroSeries(
+            parent_series=self.factory.makeDistroSeries())
+        packages = dict(
+            (user, self.factory.makeSourcePackageName())
+            for user in script_users)
+        transaction.commit()
+        for user in script_users:
+            self.layer.switchDbUser(user)
+            try:
+                create_job(derived_series, packages[user])
+            except ProgrammingError, e:
+                self.assertTrue(
+                    False,
+                    "Database role %s was unable to create a job.  "
+                    "Error was: %s" % (user, e))
+
+        # The test is that we get here without exceptions.
+        pass