launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-no-delete-with-pending-builds into lp:launchpad.

Commit message:
Forbid deleting snap packages with pending builds.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-no-delete-with-pending-builds/+merge/294712

Forbid deleting snap packages with pending builds.

This is a compromise between the original rather over-the-top restriction (no deletion of packages that have ever had any builds) and the current state, which results in build farm chaos if somebody happens to delete a snap that has pending builds because now we end up with BuildQueue and BuildFarmJob rows that have no specific build.  I considered dealing with the pending-build case instead, but it's potentially rather slow, especially if builds are actually running in which case we'd have to arrange to finish the deletion after the builders finish cancelling.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-no-delete-with-pending-builds into lp:launchpad.

=== modified file 'lib/lp/snappy/browser/configure.zcml'
--- lib/lp/snappy/browser/configure.zcml	2016-05-06 11:54:18 +0000
+++ lib/lp/snappy/browser/configure.zcml	2016-05-14 15:34:09 +0000
@@ -71,7 +71,7 @@
             class="lp.snappy.browser.snap.SnapDeleteView"
             permission="launchpad.Edit"
             name="+delete"
-            template="../../app/templates/generic-edit.pt" />
+            template="../templates/snap-delete.pt" />
         <adapter
             provides="lp.services.webapp.interfaces.IBreadcrumb"
             for="lp.snappy.interfaces.snap.ISnap"

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2016-05-06 09:45:45 +0000
+++ lib/lp/snappy/browser/snap.py	2016-05-14 15:34:09 +0000
@@ -514,6 +514,10 @@
 
     field_names = []
 
+    @property
+    def has_pending_builds(self):
+        return not self.context.pending_builds.is_empty()
+
     @action('Delete snap package', name='delete')
     def delete_action(self, action, data):
         owner = self.context.owner

=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py	2016-05-06 09:45:45 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py	2016-05-14 15:34:09 +0000
@@ -594,12 +594,24 @@
         self.assertEqual(owner_url + "/+snaps", browser.url)
         self.assertRaises(NotFound, browser.open, snap_url)
 
+    def test_delete_snap_with_pending_builds(self):
+        # A snap package with pending builds cannot be deleted.
+        self.useFixture(FakeLogger())
+        snap = self.factory.makeSnap(registrant=self.person, owner=self.person)
+        self.factory.makeSnapBuild(snap=snap, status=BuildStatus.NEEDSBUILD)
+        browser = self.getViewBrowser(snap, user=self.person)
+        browser.getLink("Delete snap package").click()
+        self.assertIn("This snap package cannot be deleted", browser.contents)
+        self.assertRaises(
+            LookupError, browser.getControl, "Delete snap package")
+
     def test_delete_snap_with_builds(self):
-        # A snap package with builds can be deleted.
+        # A snap package with completed builds can be deleted.
         self.useFixture(FakeLogger())
         snap = self.factory.makeSnap(registrant=self.person, owner=self.person)
-        build = self.factory.makeSnapBuild(snap=snap)
-        self.factory.makeSnapFile(snapbuild=build)
+        for status in (BuildStatus.FULLYBUILT, BuildStatus.CANCELLED):
+            build = self.factory.makeSnapBuild(snap=snap, status=status)
+            self.factory.makeSnapFile(snapbuild=build)
         snap_url = canonical_url(snap)
         owner_url = canonical_url(self.person)
         browser = self.getViewBrowser(snap, user=self.person)

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2016-05-11 00:00:47 +0000
+++ lib/lp/snappy/interfaces/snap.py	2016-05-14 15:34:09 +0000
@@ -7,6 +7,7 @@
 
 __all__ = [
     'BadSnapSearchContext',
+    'CannotDeleteSnap',
     'CannotModifySnapProcessor',
     'DuplicateSnapName',
     'ISnap',
@@ -197,6 +198,11 @@
             "Snap contains private information and cannot be public.")
 
 
+@error_status(httplib.BAD_REQUEST)
+class CannotDeleteSnap(Exception):
+    """This snap package cannot be deleted."""
+
+
 class BadSnapSearchContext(Exception):
     """The context is not valid for a snap package search."""
 
@@ -302,7 +308,7 @@
     @export_destructor_operation()
     @operation_for_version("devel")
     def destroySelf():
-        """Delete this snap package, provided that it has no builds."""
+        """Delete this snap package, provided that it has no pending builds."""
 
 
 class ISnapEditableAttributes(IHasOwner):

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2016-05-11 00:00:47 +0000
+++ lib/lp/snappy/model/snap.py	2016-05-14 15:34:09 +0000
@@ -77,6 +77,7 @@
 from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.interfaces.snap import (
     BadSnapSearchContext,
+    CannotDeleteSnap,
     CannotModifySnapProcessor,
     DuplicateSnapName,
     ISnap,
@@ -381,6 +382,9 @@
 
     def destroySelf(self):
         """See `ISnap`."""
+        if not self.pending_builds.is_empty():
+            raise CannotDeleteSnap(
+                "Cannot delete a snap package with pending builds.")
         store = IStore(Snap)
         store.find(SnapArch, SnapArch.snap == self).remove()
         # XXX cjwatson 2016-02-27 bug=322972: Requires manual SQL due to

=== added file 'lib/lp/snappy/templates/snap-delete.pt'
--- lib/lp/snappy/templates/snap-delete.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/snappy/templates/snap-delete.pt	2016-05-14 15:34:09 +0000
@@ -0,0 +1,21 @@
+<html
+  xmlns="http://www.w3.org/1999/xhtml";
+  xmlns:tal="http://xml.zope.org/namespaces/tal";
+  xmlns:metal="http://xml.zope.org/namespaces/metal";
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+  metal:use-macro="view/macro:page/main_only"
+  i18n:domain="launchpad">
+<body>
+
+  <div metal:fill-slot="main">
+    <tal:has-builds condition="view/has_pending_builds">
+      This snap package cannot be deleted as it has pending builds.
+    </tal:has-builds>
+
+    <tal:has-no-builds condition="not: view/has_pending_builds">
+      <div metal:use-macro="context/@@launchpad_form/form"/>
+    </tal:has-no-builds>
+  </div>
+
+</body>
+</html>

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2016-05-06 09:45:45 +0000
+++ lib/lp/snappy/tests/test_snap.py	2016-05-14 15:34:09 +0000
@@ -38,6 +38,7 @@
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.snappy.interfaces.snap import (
     BadSnapSearchContext,
+    CannotDeleteSnap,
     CannotModifySnapProcessor,
     ISnap,
     ISnapSet,
@@ -373,25 +374,45 @@
         super(TestSnapDeleteWithBuilds, self).setUp()
         self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
 
+    def test_delete_with_pending_builds(self):
+        # A snap package with pending builds cannot be deleted.
+        owner = self.factory.makePerson()
+        distroseries = self.factory.makeDistroSeries()
+        snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, distroseries=distroseries,
+            name=u"condemned")
+        self.factory.makeSnapBuild(snap=snap, status=BuildStatus.NEEDSBUILD)
+        self.assertTrue(getUtility(ISnapSet).exists(owner, u"condemned"))
+        with person_logged_in(snap.owner):
+            self.assertRaises(CannotDeleteSnap, snap.destroySelf)
+        self.assertTrue(getUtility(ISnapSet).exists(owner, u"condemned"))
+
     def test_delete_with_builds(self):
-        # A snap package with builds can be deleted.  Doing so deletes all
-        # its builds and their files too.
+        # A snap package with completed builds can be deleted.  Doing so
+        # deletes all its builds and their files too.
         owner = self.factory.makePerson()
         distroseries = self.factory.makeDistroSeries()
         snap = self.factory.makeSnap(
             registrant=owner, owner=owner, distroseries=distroseries,
             name=u"condemned")
-        build = self.factory.makeSnapBuild(snap=snap)
-        snapfile = self.factory.makeSnapFile(snapbuild=build)
+        fullybuilt_build = self.factory.makeSnapBuild(
+            snap=snap, status=BuildStatus.FULLYBUILT)
+        snapfile = self.factory.makeSnapFile(snapbuild=fullybuilt_build)
         self.assertTrue(getUtility(ISnapSet).exists(owner, u"condemned"))
-        build_id = build.id
+        fullybuilt_build_id = fullybuilt_build.id
         snapfile_id = removeSecurityProxy(snapfile).id
+        cancelled_build = self.factory.makeSnapBuild(
+            snap=snap, status=BuildStatus.CANCELLED)
+        cancelled_build_id = cancelled_build.id
         with person_logged_in(snap.owner):
             snap.destroySelf()
         flush_database_caches()
         self.assertFalse(getUtility(ISnapSet).exists(owner, u"condemned"))
-        self.assertIsNone(getUtility(ISnapBuildSet).getByID(build_id))
+        self.assertIsNone(
+            getUtility(ISnapBuildSet).getByID(fullybuilt_build_id))
         self.assertIsNone(IMasterStore(SnapFile).get(SnapFile, snapfile_id))
+        self.assertIsNone(
+            getUtility(ISnapBuildSet).getByID(cancelled_build_id))
 
     def test_related_webhooks_deleted(self):
         owner = self.factory.makePerson()