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
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
