launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/change-override-forbid-release-stable into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #530020 in Launchpad itself: "change-override.py should forbid modifications of the RELEASE pocket of non-development series"
  https://bugs.launchpad.net/launchpad/+bug/530020

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/change-override-forbid-release-stable/+merge/110942

== Summary ==

Bug 530020: override changes can result in stranded publications that will never be published, since the RELEASE pockets of released DSes in the primary Ubuntu archive are immutable.

== Proposed fix ==

Move the cannotModifySuite method from the publisher down to DistroSeries so that changeOverride can use it (there are enough copies of this logic already without adding more!), and raise an exception in changeOverride if that indicates that the newly-created publication would be stranded.

== Implementation details ==

I cleaned up some useless "current = self" code in changeOverride.  This made sense before the removal of secure publication records in r7659.7.1, but now it's just more typing.

== LOC Rationale ==

+52.  I'm doing this so that I can land https://code.launchpad.net/~cjwatson/launchpad/export-change-override/+merge/109549, which is looking like it should come out at least 76 lines in credit once the old script is removed, so I think this will be OK.

== Tests ==

bin/test -vvct test_publishing.TestChangeOverride

== Demo and Q/A ==

On dogfood, attempt to run 'change-override.py -s precise -p extra base-files' and 'change-override.py -s quantal -p extra base-files'; these should respectively fail and succeed.

-- 
https://code.launchpad.net/~cjwatson/launchpad/change-override-forbid-release-stable/+merge/110942
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/change-override-forbid-release-stable into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py	2012-01-03 01:02:12 +0000
+++ lib/lp/archivepublisher/publishing.py	2012-06-19 01:34:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -302,7 +302,7 @@
         # Loop for each pocket in each distroseries:
         for distroseries in self.distro.series:
             for pocket in self.archive.getPockets():
-                if self.cannotModifySuite(distroseries, pocket):
+                if distroseries.cannotModifySuite(pocket, self.archive):
                     # We don't want to mark release pockets dirty in a
                     # stable distroseries, no matter what other bugs
                     # that precede here have dirtied it.
@@ -458,12 +458,6 @@
             package_index.close()
             di_index.close()
 
-    def cannotModifySuite(self, distroseries, pocket):
-        """Return True if the distroseries is stable and pocket is release."""
-        return (not distroseries.isUnstable() and
-                not self.archive.allowUpdatesToReleasePocket() and
-                pocket == PackagePublishingPocket.RELEASE)
-
     def checkDirtySuiteBeforePublishing(self, distroseries, pocket):
         """Last check before publishing a dirty suite.
 
@@ -471,7 +465,7 @@
         in RELEASE pocket (primary archives) we certainly have a problem,
         better stop.
         """
-        if self.cannotModifySuite(distroseries, pocket):
+        if distroseries.cannotModifySuite(pocket, self.archive):
             raise AssertionError(
                 "Oops, tainting RELEASE pocket of %s." % distroseries)
 

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2012-05-14 03:07:32 +0000
+++ lib/lp/registry/model/distroseries.py	2012-06-19 01:34:23 +0000
@@ -1642,6 +1642,12 @@
 
         return True
 
+    def cannotModifySuite(self, pocket, archive):
+        """RELEASE pockets of stable DSes in some archives are immutable."""
+        return (not self.isUnstable() and
+                not archive.allowUpdatesToReleasePocket() and
+                pocket == PackagePublishingPocket.RELEASE)
+
     @property
     def main_archive(self):
         return self.distribution.main_archive

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2012-05-21 07:34:15 +0000
+++ lib/lp/soyuz/model/publishing.py	2012-06-19 01:34:23 +0000
@@ -787,17 +787,13 @@
             raise AssertionError("changeOverride must be passed either a"
                                  " new component or new section")
 
-        # Retrieve current publishing info
-        current = self
-
         # Check there is a change to make
         if new_component is None:
-            new_component = current.component
+            new_component = self.component
         if new_section is None:
-            new_section = current.section
+            new_section = self.section
 
-        if (new_component == current.component and
-            new_section == current.section):
+        if new_component == self.component and new_section == self.section:
             return
 
         # See if the archive has changed by virtue of the component
@@ -805,18 +801,25 @@
         distribution = self.distroseries.distribution
         new_archive = distribution.getArchiveByComponent(
             new_component.name)
-        if new_archive != None and new_archive != current.archive:
+        if new_archive != None and new_archive != self.archive:
             raise ArchiveOverriderError(
                 "Overriding component to '%s' failed because it would "
                 "require a new archive." % new_component.name)
 
+        # Refuse to create new publication records that will never be
+        # published.
+        if self.distroseries.cannotModifySuite(self.pocket, self.archive):
+            raise ArchiveOverriderError(
+                "Cannot change overrides in immutable suite %s" %
+                self.distroseries.getSuite(self.pocket))
+
         return getUtility(IPublishingSet).newSourcePublication(
-            distroseries=current.distroseries,
-            sourcepackagerelease=current.sourcepackagerelease,
-            pocket=current.pocket,
+            distroseries=self.distroseries,
+            sourcepackagerelease=self.sourcepackagerelease,
+            pocket=self.pocket,
             component=new_component,
             section=new_section,
-            archive=current.archive)
+            archive=self.archive)
 
     def copyTo(self, distroseries, pocket, archive, override=None,
                create_dsd_job=True, creator=None, sponsor=None,
@@ -1202,20 +1205,17 @@
             raise AssertionError("changeOverride must be passed a new"
                                  "component, section and/or priority.")
 
-        # Retrieve current publishing info
-        current = self
-
         # Check there is a change to make
         if new_component is None:
-            new_component = current.component
+            new_component = self.component
         if new_section is None:
-            new_section = current.section
+            new_section = self.section
         if new_priority is None:
-            new_priority = current.priority
+            new_priority = self.priority
 
-        if (new_component == current.component and
-            new_section == current.section and
-            new_priority == current.priority):
+        if (new_component == self.component and
+            new_section == self.section and
+            new_priority == self.priority):
             return
 
         # See if the archive has changed by virtue of the component changing:
@@ -1227,6 +1227,13 @@
                 "Overriding component to '%s' failed because it would "
                 "require a new archive." % new_component.name)
 
+        # Refuse to create new publication records that will never be
+        # published.
+        if self.distroseries.cannotModifySuite(self.pocket, self.archive):
+            raise ArchiveOverriderError(
+                "Cannot change overrides in immutable suite %s" %
+                self.distroseries.getSuite(self.pocket))
+
         # Append the modified package publishing entry
         return BinaryPackagePublishingHistory(
             binarypackagename=self.binarypackagerelease.binarypackagename,
@@ -1234,11 +1241,11 @@
             distroarchseries=self.distroarchseries,
             status=PackagePublishingStatus.PENDING,
             datecreated=UTC_NOW,
-            pocket=current.pocket,
+            pocket=self.pocket,
             component=new_component,
             section=new_section,
             priority=new_priority,
-            archive=current.archive)
+            archive=self.archive)
 
     def copyTo(self, distroseries, pocket, archive):
         """See `BinaryPackagePublishingHistory`."""

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2012-01-20 15:42:44 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2012-06-19 01:34:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test native publication workflow for Soyuz. """
@@ -24,6 +24,7 @@
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import SourcePackageUrgency
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.services.config import config
@@ -58,6 +59,7 @@
     BinaryPackagePublishingHistory,
     SourcePackagePublishingHistory,
     )
+from lp.soyuz.scripts.changeoverride import ArchiveOverriderError
 from lp.testing import (
     StormStatementRecorder,
     TestCaseWithFactory,
@@ -1645,3 +1647,46 @@
         # archive too.
         self.assertContentEqual(
             [], getUtility(IPublishingSet).publishBinaries(**args))
+
+
+class TestChangeOverride(TestNativePublishingBase):
+    """Test that changing overrides works."""
+
+    def setUpOverride(self, status, pocket=PackagePublishingPocket.RELEASE,
+                      binary=False):
+        self.distroseries.status = status
+        if binary:
+            pub = self.getPubBinaries(pocket=pocket)[0]
+        else:
+            pub = self.getPubSource(pocket=pocket)
+        universe = getUtility(IComponentSet)["universe"]
+        return pub.changeOverride(new_component=universe)
+
+    def assertCanOverride(self, *args, **kwargs):
+        new_pub = self.setUpOverride(*args, **kwargs)
+        self.assertEqual("universe", new_pub.component.name)
+
+    def assertCannotOverride(self, *args, **kwargs):
+        self.assertRaises(
+            ArchiveOverriderError, self.setUpOverride, *args, **kwargs)
+
+    def test_changeOverride_forbids_stable_RELEASE(self):
+        # changeOverride is not allowed in the RELEASE pocket of a stable
+        # distroseries.
+        self.assertCannotOverride(SeriesStatus.CURRENT)
+        self.assertCannotOverride(SeriesStatus.CURRENT, binary=True)
+
+    def test_changeOverride_allows_development_RELEASE(self):
+        # changeOverride is allowed in the RELEASE pocket of a development
+        # distroseries.
+        self.assertCanOverride(SeriesStatus.DEVELOPMENT)
+        self.assertCanOverride(SeriesStatus.DEVELOPMENT, binary=True)
+
+    def test_changeOverride_allows_stable_PROPOSED(self):
+        # changeOverride is allowed in the PROPOSED pocket of a stable
+        # distroseries.
+        self.assertCanOverride(
+            SeriesStatus.CURRENT, pocket=PackagePublishingPocket.PROPOSED)
+        self.assertCanOverride(
+            SeriesStatus.CURRENT, pocket=PackagePublishingPocket.PROPOSED,
+            binary=True)