launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03812
[Merge] lp:~jtv/launchpad/db-bug-790761 into lp:launchpad/db-devel
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-790761 into lp:launchpad/db-devel with lp:~jtv/launchpad/db-bug-790161 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #790761 in Launchpad itself: "Package copy policy adapter"
https://bugs.launchpad.net/launchpad/+bug/790761
For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-790761/+merge/63203
= Summary =
We need to look up package-copying policies based on the database representation.
== Proposed fix ==
A new interface ICopyPolicy defines what we expect from copy policies. A new adapter gives you the ICopyPolicy for a given PackageCopyPolicy enum value.
== Pre-implementation notes ==
This was originally spec'ed to take an identifying policy string. But pushback on review for a preceding branch made us use an enumeration instead.
The ZCML registration for the adapter actually takes any DBItem value; I'm not aware of any way to adapt a specific DBEnumeration. Robert didn't seem to know of one either.
== Implementation details ==
I rearranged the copy-policy inheritance hierarchy a bit. Now that there's an interface for ICopyPolicy, there's no more need for a base class that raises "not implemented" errors from every method. Instead we can have tests verify directly whether each policy class implements the interface, which is slightly more like what a compiled language would do.
That also made it reasonable to remodel the two concrete policy classes not as one being a modification of the other, but as both delegating most of their logic to a separate base class. That should make the consequences of change slightly easier to oversee: the style rule of "don't inherit from concrete classes" is surprisingly effective at improving designs.
== Tests ==
{{{
./bin/test -vvc lp.soyuz.adapters.tests.test_copypolicy
./bin/test -vvc lp.soyuz.browser.tests.test_archive
}}}
== Demo and Q/A ==
The new stuff isn't really reachable yet. As long as we can still request package updates and asynchronous package syncs, it should be fine.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/adapters/tests/test_copypolicy.py
lib/lp/soyuz/adapters/copypolicy.py
lib/lp/soyuz/model/packagecopyjob.py
lib/lp/registry/interfaces/distroseriesdifference.py
database/schema/patch-2208-73-0.sql
lib/lp/soyuz/configure.zcml
lib/lp/soyuz/interfaces/packagecopyjob.py
lib/lp/testing/factory.py
lib/lp/soyuz/tests/test_packagecopyjob.py
lib/lp/registry/browser/tests/test_distroseries.py
lib/lp/soyuz/enums.py
lib/lp/soyuz/interfaces/copypolicy.py
lib/lp/soyuz/browser/tests/test_package_copying_mixin.py
lib/lp/registry/browser/distroseries.py
lib/lp/soyuz/browser/archive.py
--
https://code.launchpad.net/~jtv/launchpad/db-bug-790761/+merge/63203
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-790761 into lp:launchpad/db-devel.
=== modified file 'lib/lp/soyuz/adapters/copypolicy.py'
--- lib/lp/soyuz/adapters/copypolicy.py 2011-05-30 11:47:46 +0000
+++ lib/lp/soyuz/adapters/copypolicy.py 2011-06-02 08:51:24 +0000
@@ -15,57 +15,30 @@
]
+from zope.interface import implements
+
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.series import SeriesStatus
-
-
-class BaseCopyPolicy:
- """Encapsulation of the policies for copying a package in Launchpad."""
-
- @property
- def name(self):
- """The name of this policy that's used in adapter lookups."""
- raise AssertionError("Subclass must provide name property")
-
- def autoApprove(self, archive, distroseries, pocket):
- """Decide if the upload can be approved automatically or
- should be held in the queue.
-
- This should only be called for packages that are known not new.
-
- :param archive: The target `IArchive` for the upload.
- :param distroseries: The target `IDistroSeries` for the upload.
- :param pocket: The target `PackagePublishingPocket` for the upload.
- """
- raise AssertionError("Subclass must provide autoApprove")
-
- def autoApproveNew(self, archive, distroseries, pocket):
- """Decide if a previously unknown package is approved automatically
- or should be held in the queue.
-
- :param archive: The target `IArchive` for the upload.
- :param distroseries: The target `IDistroSeries` for the upload.
- :param pocket: The target `PackagePublishingPocket` for the upload.
- """
- raise AssertionError("Subclass must provide autoApproveNew")
-
- @property
- def send_email(self):
- """Whether or not the copy should send emails after completing."""
- raise AssertionError("Subclass must provide send_email")
-
-
-class InsecureCopyPolicy(BaseCopyPolicy):
- """A policy for copying from insecure sources."""
-
- name = "insecure"
+from lp.soyuz.enums import PackageCopyPolicy
+from lp.soyuz.interfaces.copypolicy import ICopyPolicy
+
+
+class BasicCopyPolicy:
+ """Useful standard copy policy.
+
+ This policy auto-approves all PPA uploads. For distribution archives it
+ auto-approves only uploads to the Release pocket, and only while the
+ series is not frozen.
+ """
def autoApproveNew(self, archive, distroseries=None, pocket=None):
+ """See `ICopyPolicy`."""
if archive.is_ppa:
return True
return False
def autoApprove(self, archive, distroseries, pocket):
+ """See `ICopyPolicy`."""
if archive.is_ppa:
return True
@@ -80,16 +53,34 @@
return False
- @property
- def send_email(self):
- return True
-
-
-class SyncCopyPolicy(InsecureCopyPolicy):
+
+class InsecureCopyPolicy(BasicCopyPolicy):
+ """A policy for copying from insecure sources."""
+ implements(ICopyPolicy)
+
+ enum_value = PackageCopyPolicy.INSECURE
+
+ send_email = True
+
+
+class SyncCopyPolicy(BasicCopyPolicy):
"""A policy for mass 'sync' copies."""
-
- name = "sync"
-
- @property
- def send_email(self):
- return False
+ implements(ICopyPolicy)
+
+ enum_value = PackageCopyPolicy.SYNC
+
+ send_email = False
+
+
+policies = [
+ InsecureCopyPolicy,
+ SyncCopyPolicy,
+ ]
+
+
+enum_to_policy = dict((policy.enum_value, policy()) for policy in policies)
+
+
+def get_icopypolicy_for_packagecopypolicy(packagecopypolicy):
+ """Look up the `ICopyPolicy` for a given `PackageCopyPolicy`."""
+ return enum_to_policy[packagecopypolicy]
=== modified file 'lib/lp/soyuz/adapters/tests/test_copypolicy.py'
--- lib/lp/soyuz/adapters/tests/test_copypolicy.py 2011-05-30 11:47:46 +0000
+++ lib/lp/soyuz/adapters/tests/test_copypolicy.py 2011-06-02 08:51:24 +0000
@@ -1,16 +1,20 @@
# Copyright 2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
+from canonical.launchpad.webapp.testing import verifyObject
from canonical.testing.layers import ZopelessDatabaseLayer
-from lp.testing import TestCaseWithFactory
-
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.series import SeriesStatus
from lp.soyuz.adapters.copypolicy import (
InsecureCopyPolicy,
SyncCopyPolicy,
)
-from lp.soyuz.enums import ArchivePurpose
+from lp.soyuz.interfaces.copypolicy import ICopyPolicy
+from lp.soyuz.enums import (
+ ArchivePurpose,
+ PackageCopyPolicy,
+ )
+from lp.testing import TestCaseWithFactory
class TestCopyPolicy(TestCaseWithFactory):
@@ -30,7 +34,8 @@
self.assertTrue(approved)
def assertUnapproved(self, archive_purpose, method):
- archive, distroseries, pocket = self._getUploadCriteria(archive_purpose)
+ archive, distroseries, pocket = self._getUploadCriteria(
+ archive_purpose)
approved = method(archive, distroseries, pocket)
self.assertFalse(approved)
@@ -42,7 +47,7 @@
cp = InsecureCopyPolicy()
self.assertApproved(ArchivePurpose.PPA, cp.autoApproveNew)
- def test_insecure_approves_existing_distro_package_to_unfrozen_release(self):
+ def test_insecure_approves_known_distro_package_to_unfrozen_release(self):
archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
distroseries = self.factory.makeDistroSeries()
pocket = PackagePublishingPocket.RELEASE
@@ -80,8 +85,6 @@
cp = SyncCopyPolicy()
self.assertFalse(cp.send_email)
- def test_policy_names(self):
- icp = InsecureCopyPolicy()
- scp = SyncCopyPolicy()
- self.assertEquals("insecure", icp.name)
- self.assertEquals("sync", scp.name)
+ def test_policies_implement_ICopyPolicy(self):
+ for policy in PackageCopyPolicy.items:
+ self.assertTrue(verifyObject(ICopyPolicy, ICopyPolicy(policy)))
=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml 2011-05-29 21:18:09 +0000
+++ lib/lp/soyuz/configure.zcml 2011-06-02 08:51:24 +0000
@@ -919,6 +919,18 @@
<allow interface=".interfaces.packagecopyjob.IPlainPackageCopyJob" />
</class>
+ <!-- PackageCopyPolicy -->
+ <class class=".adapters.copypolicy.InsecureCopyPolicy">
+ <allow interface=".interfaces.copypolicy.ICopyPolicy"/>
+ </class>
+ <class class=".adapters.copypolicy.SyncCopyPolicy">
+ <allow interface=".interfaces.copypolicy.ICopyPolicy"/>
+ </class>
+ <adapter
+ for="lazr.enum.DBItem"
+ provides="lp.soyuz.interfaces.copypolicy.ICopyPolicy"
+ factory="lp.soyuz.adapters.copypolicy.get_icopypolicy_for_packagecopypolicy"/>
+
<!-- PackageCopyRequestSet -->
<securedutility
class="lp.soyuz.model.packagecopyrequest.PackageCopyRequestSet"
=== modified file 'lib/lp/soyuz/enums.py'
--- lib/lp/soyuz/enums.py 2011-06-02 08:51:16 +0000
+++ lib/lp/soyuz/enums.py 2011-06-02 08:51:24 +0000
@@ -236,7 +236,10 @@
class PackageCopyPolicy(DBEnumeratedType):
- """Package copying policy."""
+ """Package copying policy.
+
+ Each of these is associated with one `ICopyPolicy`.
+ """
INSECURE = DBItem(1, """
Copy from insecure source.
=== added file 'lib/lp/soyuz/interfaces/copypolicy.py'
--- lib/lp/soyuz/interfaces/copypolicy.py 1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/interfaces/copypolicy.py 2011-06-02 08:51:24 +0000
@@ -0,0 +1,53 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Package copy policies."""
+
+__metaclass__ = type
+__all__ = [
+ 'ICopyPolicy',
+ ]
+
+
+from zope.interface import Interface
+from zope.schema import (
+ Bool,
+ Choice,
+ )
+
+from canonical.launchpad import _
+from lp.soyuz.enums import PackageCopyPolicy
+
+
+class ICopyPolicy(Interface):
+ """Policies for copying packages, as enumerated by `PackageCopyPolicy`."""
+
+ enum_value = Choice(
+ title=_("PackageCopyPolicy number associated with this policy."),
+ values=PackageCopyPolicy, readonly=True, required=True)
+
+ send_email = Bool(
+ title=_("Should completion of this copy be announced by email?"),
+ readonly=True, required=True)
+
+ def autoApprove(archive, distroseries, pocket):
+ """Can this upload of a known package be approved automatically?
+
+ This should only be called for packages that are known not new.
+
+ :param archive: The target `IArchive` for the upload.
+ :param distroseries: The target `IDistroSeries` for the upload.
+ :param pocket: The target `PackagePublishingPocket` for the upload.
+ :return: True if the upload can be auto-approved, or False if it
+ should be held in the queue.
+ """
+
+ def autoApproveNew(archive, distroseries, pocket):
+ """Can this upload of a new package be approved automatically?
+
+ :param archive: The target `IArchive` for the upload.
+ :param distroseries: The target `IDistroSeries` for the upload.
+ :param pocket: The target `PackagePublishingPocket` for the upload.
+ :return: True if the upload can be auto-approved, or False if it
+ should be held in the queue.
+ """
=== modified file 'lib/lp/soyuz/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py 2011-06-02 08:51:16 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py 2011-06-02 08:51:24 +0000
@@ -86,7 +86,8 @@
def create(cls, package_name, source_archive,
target_archive, target_distroseries, target_pocket,
- include_binaries=False, package_version=None):
+ include_binaries=False, package_version=None,
+ copy_policy=None):
"""Create a new `IPackageCopyJob`.
:param package_name: The name of the source package to copy.
@@ -100,6 +101,7 @@
:param include_binaries: See `do_copy`.
:param package_version: The version string for the package version
that is to be copied.
+ :param copy_policy: Applicable `PackageCopyPolicy`.
"""
def getActiveJobs(target_archive):
