launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03813
[Merge] lp:~jtv/launchpad/db-bug-791737 into lp:launchpad/db-devel
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-791737 into lp:launchpad/db-devel with lp:~jtv/launchpad/db-bug-790761 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #791737 in Launchpad itself: "Set copy_policy on PlainPackageCopyJobs."
https://bugs.launchpad.net/launchpad/+bug/791737
For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-791737/+merge/63220
This adds a small piece to the package-copy-policy puzzle: when creating a PlainPackageCopyJob, set its copy_policy to one of two available PackageCopyPolicy enum values. The default is the "insecure" policy. (Providing a default seemed slightly risky to me, so I looked for forgotten call sites by making the parameter default to None, adding an assertion that a different value had actually been passed, and running tests).
Each PackageCopyPolicy value refers to a respective ICopyPolicy implementation. For convenience I gave the job type a method that looks up this policy implementation.
You may notice that the copy_policy field is slightly weird. In storage terms, it's on the PackageCopyJob base class and is allowed to be None. But in interface terms it's on the more specialized IPlainPackageCopyJob interface where it's not allowed to be None. It's not, as you might expect, on the parent IPackageCopyJob interface. It wouldn't really need to be on the interface, except the PlainPackageCopyJob class only gets its actual IPlainPackageCopyJob data attributes by delegating to PackageCopyJob. So copy_policy needs to be added to the interface in order for PlainPackageCopyJob to get a self.copy_policy.
Finally, Julian asked me to rename SyncPolicy to MassSyncPolicy. This will be less confusing as the "package upgrade" operation is to be renamed to "mass sync." (The current situation, confusingly, has SyncPolicy for upgrades and InsecurePolicy for syncs).
To Q/A this, create IPlainPackageCopyJobs through the mass-sync option as well as by requesting more package syncs manually on DistroSeries:+localpackagediffs than the max_synchronous_syncs feature flag allows. The former should have a policy of MASS_SYNC (or 2 numerically) whereas the latter will have INSECURE (or 1 numerically).
No lint.
Jeroen
--
https://code.launchpad.net/~jtv/launchpad/db-bug-791737/+merge/63220
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-791737 into lp:launchpad/db-devel.
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py 2011-06-02 11:24:35 +0000
+++ lib/lp/registry/browser/distroseries.py 2011-06-02 11:24:39 +0000
@@ -104,6 +104,7 @@
from lp.services.worlddata.interfaces.language import ILanguageSet
from lp.soyuz.browser.archive import PackageCopyingMixin
from lp.soyuz.browser.packagesearch import PackageSearchViewBase
+from lp.soyuz.enums import PackageCopyPolicy
from lp.soyuz.interfaces.distributionjob import (
IDistroSeriesDifferenceJobSource,
)
@@ -1084,7 +1085,8 @@
dsd.source_package_name.name,
dsd.parent_series.main_archive, target_archive,
target_distroseries, PackagePublishingPocket.UPDATES,
- package_version=dsd.parent_source_version)
+ package_version=dsd.parent_source_version,
+ copy_policy=PackageCopyPolicy.SYNC)
self.request.response.addInfoNotification(
(u"Upgrades of {context.displayname} packages have been "
=== modified file 'lib/lp/soyuz/adapters/copypolicy.py'
--- lib/lp/soyuz/adapters/copypolicy.py 2011-06-02 11:24:35 +0000
+++ lib/lp/soyuz/adapters/copypolicy.py 2011-06-02 11:24:39 +0000
@@ -9,9 +9,9 @@
__metaclass__ = type
+# All of this module's functionality can be reached through the
+# ICopyPolicy adapter.
__all__ = [
- "InsecureCopyPolicy",
- "SyncCopyPolicy",
]
@@ -63,18 +63,18 @@
send_email = True
-class SyncCopyPolicy(BasicCopyPolicy):
+class MassSyncCopyPolicy(BasicCopyPolicy):
"""A policy for mass 'sync' copies."""
implements(ICopyPolicy)
- enum_value = PackageCopyPolicy.SYNC
+ enum_value = PackageCopyPolicy.MASS_SYNC
send_email = False
policies = [
InsecureCopyPolicy,
- SyncCopyPolicy,
+ MassSyncCopyPolicy,
]
=== modified file 'lib/lp/soyuz/adapters/tests/test_copypolicy.py'
--- lib/lp/soyuz/adapters/tests/test_copypolicy.py 2011-06-02 11:24:35 +0000
+++ lib/lp/soyuz/adapters/tests/test_copypolicy.py 2011-06-02 11:24:39 +0000
@@ -7,7 +7,7 @@
from lp.registry.interfaces.series import SeriesStatus
from lp.soyuz.adapters.copypolicy import (
InsecureCopyPolicy,
- SyncCopyPolicy,
+ MassSyncCopyPolicy,
)
from lp.soyuz.interfaces.copypolicy import ICopyPolicy
from lp.soyuz.enums import (
@@ -82,7 +82,7 @@
self.assertTrue(cp.send_email)
def test_sync_does_not_send_emails(self):
- cp = SyncCopyPolicy()
+ cp = MassSyncCopyPolicy()
self.assertFalse(cp.send_email)
def test_policies_implement_ICopyPolicy(self):
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py 2011-06-02 11:24:35 +0000
+++ lib/lp/soyuz/browser/archive.py 2011-06-02 11:24:39 +0000
@@ -134,6 +134,7 @@
ArchivePermissionType,
ArchivePurpose,
ArchiveStatus,
+ PackageCopyPolicy,
PackagePublishingStatus,
)
from lp.soyuz.interfaces.archive import (
@@ -1308,7 +1309,8 @@
job_source.create(
spph.source_package_name, spph.archive, dest_archive, dest_series,
dest_pocket, include_binaries=include_binaries,
- package_version=spph.sourcepackagerelease.version)
+ package_version=spph.sourcepackagerelease.version,
+ copy_policy=PackageCopyPolicy.INSECURE)
return structured("""
<p>Requested sync of %s packages.</p>
=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml 2011-06-02 11:24:35 +0000
+++ lib/lp/soyuz/configure.zcml 2011-06-02 11:24:39 +0000
@@ -923,7 +923,7 @@
<class class=".adapters.copypolicy.InsecureCopyPolicy">
<allow interface=".interfaces.copypolicy.ICopyPolicy"/>
</class>
- <class class=".adapters.copypolicy.SyncCopyPolicy">
+ <class class=".adapters.copypolicy.MassSyncCopyPolicy">
<allow interface=".interfaces.copypolicy.ICopyPolicy"/>
</class>
<adapter
=== modified file 'lib/lp/soyuz/enums.py'
--- lib/lp/soyuz/enums.py 2011-06-02 11:24:35 +0000
+++ lib/lp/soyuz/enums.py 2011-06-02 11:24:39 +0000
@@ -247,10 +247,10 @@
This is the default.
""")
- SYNC = DBItem(2, """
- Package sync.
+ MASS_SYNC = DBItem(2, """
+ Mass package sync.
- This policy applies when synchronizing packages en masses.
+ This policy applies when synchronizing packages en masse.
""")
=== modified file 'lib/lp/soyuz/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py 2011-06-02 11:24:35 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py 2011-06-02 11:24:39 +0000
@@ -21,6 +21,7 @@
)
from zope.schema import (
Bool,
+ Choice,
Int,
TextLine,
)
@@ -32,6 +33,7 @@
IJobSource,
IRunnableJob,
)
+from lp.soyuz.enums import PackageCopyPolicy
from lp.soyuz.interfaces.archive import IArchive
@@ -84,10 +86,10 @@
class IPlainPackageCopyJobSource(IJobSource):
"""An interface for acquiring `IPackageCopyJobs`."""
- def create(cls, package_name, source_archive,
+ def create(package_name, source_archive,
target_archive, target_distroseries, target_pocket,
include_binaries=False, package_version=None,
- copy_policy=None):
+ copy_policy=PackageCopyPolicy.INSECURE):
"""Create a new `IPackageCopyJob`.
:param package_name: The name of the source package to copy.
@@ -136,3 +138,7 @@
include_binaries = Bool(
title=_("Copy binaries"),
required=False, readonly=True)
+
+ copy_policy = Choice(
+ title=_("Applicable copy policy"),
+ values=PackageCopyPolicy, required=True, readonly=True)
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py 2011-06-02 11:24:35 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py 2011-06-02 11:24:39 +0000
@@ -47,6 +47,7 @@
from lp.services.job.runner import BaseRunnableJob
from lp.soyuz.enums import PackageCopyPolicy
from lp.soyuz.interfaces.archive import CannotCopy
+from lp.soyuz.interfaces.copypolicy import ICopyPolicy
from lp.soyuz.interfaces.packagecopyjob import (
IPackageCopyJob,
IPlainPackageCopyJob,
@@ -159,6 +160,11 @@
])
return vars
+ @property
+ def copy_policy(self):
+ """See `PlainPackageCopyJob`."""
+ return self.context.copy_policy
+
class PlainPackageCopyJob(PackageCopyJobDerived):
"""Job that copies a package from one archive to another."""
@@ -177,7 +183,7 @@
def create(cls, package_name, source_archive,
target_archive, target_distroseries, target_pocket,
include_binaries=False, package_version=None,
- copy_policy=None):
+ copy_policy=PackageCopyPolicy.INSECURE):
"""See `IPlainPackageCopyJobSource`."""
assert package_version is not None, "No package version specified."
metadata = {
@@ -335,3 +341,7 @@
if self.include_binaries:
parts.append(", including binaries")
return "<%s>" % "".join(parts)
+
+ def getPolicyImplementation(self):
+ """Return the `ICopyPolicy` applicable to this job."""
+ return ICopyPolicy(self.copy_policy)
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2011-06-02 11:24:35 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2011-06-02 11:24:39 +0000
@@ -10,6 +10,7 @@
from canonical.config import config
from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.launchpad.webapp.testing import verifyObject
from canonical.testing import LaunchpadZopelessLayer
from lp.registry.model.distroseriesdifferencecomment import (
DistroSeriesDifferenceComment,
@@ -19,6 +20,7 @@
from lp.services.job.interfaces.job import JobStatus
from lp.soyuz.enums import (
ArchivePurpose,
+ PackageCopyPolicy,
SourcePackageFormat,
)
from lp.soyuz.model.distroseriesdifferencejob import (
@@ -27,6 +29,7 @@
from lp.soyuz.interfaces.archive import CannotCopy
from lp.soyuz.interfaces.packagecopyjob import (
IPackageCopyJob,
+ IPlainPackageCopyJob,
IPlainPackageCopyJobSource,
)
from lp.soyuz.interfaces.publishing import PackagePublishingStatus
@@ -51,7 +54,7 @@
class LocalTestHelper:
"""Put test helpers that want to be in the test classes here."""
- def makeJob(self, dsd=None):
+ def makeJob(self, dsd=None, **kwargs):
"""Create a `PlainPackageCopyJob` that would resolve `dsd`."""
if dsd is None:
dsd = self.factory.makeDistroSeriesDifference()
@@ -62,7 +65,7 @@
return getUtility(IPlainPackageCopyJobSource).create(
dsd.source_package_name.name, source_archive, target_archive,
target_distroseries, target_pocket,
- package_version=dsd.parent_source_version)
+ package_version=dsd.parent_source_version, **kwargs)
def runJob(self, job):
"""Helper to switch to the right DB user and run the job."""
@@ -76,6 +79,14 @@
layer = LaunchpadZopelessLayer
+ def test_job_implements_IPlainPackageCopyJob(self):
+ job = self.makeJob()
+ self.assertTrue(verifyObject(IPlainPackageCopyJob, job))
+
+ def test_job_source_implements_IPlainPackageCopyJobSource(self):
+ job_source = getUtility(IPlainPackageCopyJobSource)
+ self.assertTrue(verifyObject(IPlainPackageCopyJobSource, job_source))
+
def test_create(self):
# A PackageCopyJob can be created and stores its arguments.
distroseries = self.factory.makeDistroSeries()
@@ -86,7 +97,8 @@
package_name="foo", source_archive=archive1,
target_archive=archive2, target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- package_version="1.0-1", include_binaries=False)
+ package_version="1.0-1", include_binaries=False,
+ copy_policy=PackageCopyPolicy.MASS_SYNC)
self.assertProvides(job, IPackageCopyJob)
self.assertEquals(archive1.id, job.source_archive_id)
self.assertEquals(archive1, job.source_archive)
@@ -97,6 +109,7 @@
self.assertEqual("foo", job.package_name)
self.assertEqual("1.0-1", job.package_version)
self.assertEquals(False, job.include_binaries)
+ self.assertEquals(PackageCopyPolicy.MASS_SYNC, job.copy_policy)
def test_getActiveJobs(self):
# getActiveJobs() can retrieve all active jobs for an archive.
@@ -424,6 +437,16 @@
naked_job = removeSecurityProxy(self.makeJob(dsd))
self.assertContentEqual([dsd], naked_job.findMatchingDSDs())
+ def test_getPolicyImplementation_returns_policy(self):
+ # getPolicyImplementation returns the ICopyPolicy that was
+ # chosen for the job.
+ dsd = self.factory.makeDistroSeriesDifference()
+ for policy in PackageCopyPolicy.items:
+ naked_job = removeSecurityProxy(
+ self.makeJob(dsd, copy_policy=policy))
+ self.assertEqual(
+ policy, naked_job.getPolicyImplementation().enum_value)
+
class TestPlainPackageCopyJobPrivileges(TestCaseWithFactory, LocalTestHelper):
"""Test that `PlainPackageCopyJob` has the privileges it needs.
