launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02927
[Merge] lp:~jtv/launchpad/bug-733132 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-733132 into lp:launchpad with lp:~jtv/launchpad/bug-730460-job-class as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #733132 in Launchpad itself: "Feature flag for DistroSeriesDifferenceJob"
https://bugs.launchpad.net/launchpad/+bug/733132
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-733132/+merge/53009
= 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
--
https://code.launchpad.net/~jtv/launchpad/bug-733132/+merge/53009
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-733132 into lp:launchpad.
=== 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 12:47:01 +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 12:46:59 +0000
+++ lib/lp/soyuz/model/distroseriesdifferencejob.py 2011-03-11 12:47:01 +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 = 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):
+ 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 12:46:59 +0000
+++ lib/lp/soyuz/tests/test_distroseriesdifferencejob.py 2011-03-11 12:47:01 +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,
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: 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: ''}))
+ 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
