launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/ppa-permadeath into lp:launchpad.

Commit message:
Prevent reenablement of deleted PPAs, and rename them after they're gone from disk to free up the namespace.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/ppa-permadeath/+merge/149462

Prevent reenablement of deleted PPAs, and rename them after they're gone from disk to free up the namespace.

Once this is deployed we'll need to clean up the DB and filesystem to ensure that all DELETED PPAs are really gone and disabled, setting any reenabled PPAs back to ACTIVE. Then we can add a DB constraint.
-- 
https://code.launchpad.net/~wgrant/launchpad/ppa-permadeath/+merge/149462
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/ppa-permadeath into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py	2013-02-20 01:42:49 +0000
+++ lib/lp/archivepublisher/publishing.py	2013-02-20 04:17:24 +0000
@@ -52,6 +52,7 @@
     BinaryPackageFormat,
     PackagePublishingStatus,
     )
+from lp.soyuz.interfaces.archive import NoSuchPPA
 from lp.soyuz.interfaces.publishing import (
     active_publishing_status,
     IPublishingSet,
@@ -745,7 +746,7 @@
         Any errors encountered while removing the archive from disk will
         be caught and an OOPS report generated.
         """
-
+        assert self.archive.is_ppa
         root_dir = os.path.join(
             self._config.distroroot, self.archive.owner.name,
             self.archive.name)
@@ -790,3 +791,19 @@
 
         self.archive.status = ArchiveStatus.DELETED
         self.archive.publish = False
+
+        # Now that it's gone from disk we can rename the archive to free
+        # up the namespace.
+        new_name = base_name = '%s-deletedppa' % self.archive.name
+        count = 1
+        while True:
+            try:
+                self.archive.owner.getPPAByName(new_name)
+            except NoSuchPPA:
+                break
+            new_name = '%s%d' % (base_name, count)
+            count += 1
+        self.archive.name = new_name
+        self.log.info(
+            "Renamed deleted archive '%s/%s'.", self.archive.owner.name,
+            self.archive.name)

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2013-02-20 01:42:49 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2013-02-20 04:17:24 +0000
@@ -147,7 +147,7 @@
         ubuntu_team = getUtility(IPersonSet).getByName('ubuntu-team')
         test_archive = getUtility(IArchiveSet).new(
             distribution=self.ubuntutest, owner=ubuntu_team,
-            purpose=ArchivePurpose.PPA)
+            purpose=ArchivePurpose.PPA, name='testing')
 
         # Create some source and binary publications, including an
         # orphaned NBS binary.
@@ -185,6 +185,7 @@
         self.assertFalse(os.path.exists(publisher._config.metaroot))
         self.assertEqual(ArchiveStatus.DELETED, test_archive.status)
         self.assertEqual(False, test_archive.publish)
+        self.assertEqual(u'testing-deletedppa', test_archive.name)
 
         # All of the archive's active publications have been marked
         # DELETED, and dateremoved has been set early because they've
@@ -226,6 +227,15 @@
         self.assertNotIn('WARNING', logger.getLogBuffer())
         self.assertNotIn('ERROR', logger.getLogBuffer())
 
+    def testDeletingPPARename(self):
+        a1 = self.factory.makeArchive(purpose=ArchivePurpose.PPA, name='test')
+        getPublisher(a1, None, self.logger).deleteArchive()
+        self.assertEqual('test-deletedppa', a1.name)
+        a2 = self.factory.makeArchive(
+            purpose=ArchivePurpose.PPA, name='test', owner=a1.owner)
+        getPublisher(a2, None, self.logger).deleteArchive()
+        self.assertEqual('test-deletedppa1', a2.name)
+
     def testPublishPartner(self):
         """Test that a partner package is published to the right place."""
         archive = self.ubuntutest.getArchiveByComponent('partner')

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2013-02-18 03:47:21 +0000
+++ lib/lp/soyuz/browser/archive.py	2013-02-20 04:17:24 +0000
@@ -1978,8 +1978,14 @@
         return canonical_url(self.context)
 
     def validate_save(self, action, data):
-        """Default save validation does nothing."""
-        pass
+        """Check that we're not reenabling a deleted archive.."""
+        form.getWidgetsData(self.widgets, 'field', data)
+
+        # Deleted PPAs can't be reactivated.
+        if ((data.get('enabled') or data.get('publish'))
+            and not self.context.is_active):
+            self.setFieldError(
+                "enabled", "Deleted PPAs can't be enabled.")
 
 
 class ArchiveEditView(BaseArchiveEditView):
@@ -2054,7 +2060,7 @@
         If the archive is private and the buildd secret is not set it will be
         generated.
         """
-        form.getWidgetsData(self.widgets, 'field', data)
+        super(ArchiveAdminView, self).validate_save(action, data)
 
         if data.get('private') != self.context.private:
             # The privacy is being switched.

=== modified file 'lib/lp/soyuz/doc/archive-deletion.txt'
--- lib/lp/soyuz/doc/archive-deletion.txt	2013-02-18 04:45:52 +0000
+++ lib/lp/soyuz/doc/archive-deletion.txt	2013-02-20 04:17:24 +0000
@@ -59,3 +59,13 @@
 
     >>> print archive.status.name
     DELETING
+
+Once deleted the archive can't be reenabled.
+
+    >>> archive.enable()
+    Traceback (most recent call last):
+    ...
+    AssertionError: Deleted archives can't be enabled.
+
+    >>> print archive.enabled
+    False

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2013-02-20 02:34:58 +0000
+++ lib/lp/soyuz/model/archive.py	2013-02-20 04:17:24 +0000
@@ -224,7 +224,11 @@
         Also assert the name is valid when set via an unproxied object.
         """
         if not self._SO_creating:
-            assert self.is_copy, "Only COPY archives can be renamed."
+            renamable = (
+                self.is_copy or
+                (self.is_ppa and self.status == ArchiveStatus.DELETED))
+            assert renamable, (
+                "Only COPY archives and deleted PPAs can be renamed.")
         assert valid_name(value), "Invalid name given to unproxied object."
         return value
 
@@ -1965,6 +1969,7 @@
     def enable(self):
         """See `IArchive`."""
         assert self._enabled == False, "This archive is already enabled."
+        assert self.is_active, "Deleted archives can't be enabled."
         self._enabled = True
         self._setBuildStatuses(JobStatus.WAITING)
 

=== modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt'
--- lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt	2012-12-10 13:43:47 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt	2013-02-20 04:17:24 +0000
@@ -818,3 +818,22 @@
     ...
     LinkNotFoundError
 
+Even if someone URL-hacks to the edit form, it's not possible to
+reenable the PPA or turn on publishing.
+
+    >>> no_priv_browser.open(
+    ...     "http://launchpad.dev/~no-priv/+archive/ppa/+edit";)
+    >>> no_priv_browser.getControl(name='field.enabled').value = True
+    >>> no_priv_browser.getControl('Save').click()
+    >>> "Deleted PPAs can't be enabled." in no_priv_browser.contents
+    True
+    >>> no_priv_browser.open(
+    ...     "http://launchpad.dev/~no-priv/+archive/ppa/+edit";)
+    >>> no_priv_browser.getControl(name='field.publish').value = True
+    >>> no_priv_browser.getControl('Save').click()
+    >>> "Deleted PPAs can't be enabled." in no_priv_browser.contents
+    True
+    >>> no_priv_browser.getLink('Cancel').click()
+    >>> print extract_text(
+    ...     first_tag_by_class(no_priv_browser.contents, 'warning message'))
+    This PPA has been deleted.