Merge lp:~julian-edwards/launchpad/cancel-build-bug-173018-ui-part3 into lp:launchpad

[Julian Edwards]

[r=gmb][bug=173018][incr] Add API functionality to cancel virtual builds.

Add BinaryPackageBuild.cancel() and can_be_cancelled property, and export both to the webservice.

[Graham Binns (gmb)]

Hi Julian,

Nice branch, just the one comment, otherwise r=me

186 + def test_cancel_not_in_progress(self):
187 + # Testing the cancel() method for a pending build.

195 + def test_cancel_in_progress(self):
196 + # Testing the cancel() method for a building build.

E_LAZY_DEVELOPER: I can figure out what you're testing from the method names - the comments more-or-less repeat that information - but I don't know what the expected behaviour is unless I read the code. Please update the comments so that they tell me what _should_ happen.

review: Approve (code)

[lp:~julian-edwards/launchpad/cancel-build-bug-173018-ui-part3]

14224. By Julian Edwards on 2011-11-01

suggestion from gmb in review

=== modified file 'lib/lp/soyuz/interfaces/binarypackagebuild.py'
--- lib/lp/soyuz/interfaces/binarypackagebuild.py	2011-06-16 20:12:00 +0000
+++ lib/lp/soyuz/interfaces/binarypackagebuild.py	2011-11-01 14:12:30 +0000
@@ -27,6 +27,7 @@
     export_as_webservice_entry,
     export_write_operation,
     exported,
+    operation_for_version,
     operation_parameters,
     )
 from lazr.restful.fields import Reference
@@ -119,6 +120,12 @@
             description=_(
                 "Whether or not this build record can be retried.")))
 
+    can_be_cancelled = exported(
+        Bool(
+            title=_("Can Be Cancelled"), required=False, readonly=True,
+            description=_(
+                "Whether or not this build record can be cancelled.")))
+
     is_virtualized = Attribute(
         "Whether or not this build requires a virtual build host or not.")
 
@@ -223,6 +230,22 @@
         non-scored BuildQueue entry is created for it.
         """
 
+    @export_write_operation()
+    @operation_for_version("devel")
+    def cancel():
+        """Cancel the build if it is either pending or in progress.
+
+        Call the can_be_cancelled() method prior to this one to find out if
+        cancelling the build is possible.
+
+        If the build is in progress, it is marked as CANCELLING until the
+        buildd manager terminates the build and marks it CANCELLED. If the
+        build is not in progress, it is marked CANCELLED immediately and is
+        removed from the build queue.
+
+        If the build is not in a cancellable state, this method is a no-op.
+        """
+
 
 class IBinaryPackageBuildAdmin(Interface):
     """A Build interface for items requiring launchpad.Admin."""
 
=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2011-09-23 15:46:11 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2011-11-01 14:12:30 +0000
@@ -349,6 +349,20 @@
         """See `IBuild`."""
         return self.status is BuildStatus.NEEDSBUILD
 
+    @property
+    def can_be_cancelled(self):
+        """See `IBuild`."""
+        if not self.buildqueue_record:
+            return False
+        if self.buildqueue_record.virtualized is False:
+            return False
+
+        cancellable_statuses = [
+            BuildStatus.BUILDING,
+            BuildStatus.NEEDSBUILD,
+            ]
+        return self.status in cancellable_statuses
+
     def retry(self):
         """See `IBuild`."""
         assert self.can_be_retried, "Build %s cannot be retried" % self.id
@@ -377,6 +391,20 @@
         else:
             return self.buildqueue_record.lastscore
 
+    def cancel(self):
+        """See `IBinaryPackageBuild`."""
+        if not self.can_be_cancelled:
+            return
+
+        # If the build is currently building we need to tell the
+        # buildd-manager to terminate it.
+        if self.status == BuildStatus.BUILDING:
+            self.status = BuildStatus.CANCELLING
+            return
+
+        # Otherwise we can cancel it here.
+        self.buildqueue_record.cancel()
+
     def makeJob(self):
         """See `IBuildFarmJob`."""
         store = Store.of(self)
 
=== modified file 'lib/lp/soyuz/stories/webservice/xx-builds.txt'
--- lib/lp/soyuz/stories/webservice/xx-builds.txt	2011-05-04 02:45:25 +0000
+++ lib/lp/soyuz/stories/webservice/xx-builds.txt	2011-11-01 14:12:30 +0000
@@ -42,6 +42,7 @@
     >>> pprint_entry(builds['entries'][0])
     arch_tag: u'i386'
     archive_link: u'http://.../beta/~cprov/+archive/ppa'
+    can_be_cancelled: False
     can_be_rescored: False
     can_be_retried: True
     changesfile_url: None
@@ -71,6 +72,7 @@
     archive_link: u'http://.../~cprov/+archive/ppa'
     build_log_url: u'http://.../~cprov/+archive/ppa/+build/26/+files/netapplet-1.0.0.tar.gz'
     buildstate: u'Failed to build'
+    can_be_cancelled: False
     can_be_rescored: False
     can_be_retried: True
     changesfile_url: None
 
=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2011-05-04 06:37:50 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2011-11-01 14:12:30 +0000
@@ -15,7 +15,15 @@
 
 from twisted.trial.unittest import TestCase as TrialTestCase
 
-from canonical.testing.layers import LaunchpadZopelessLayer
+from canonical.launchpad.testing.pages import (
+    webservice_for_person,
+    )
+from canonical.launchpad.webapp.interaction import ANONYMOUS
+from canonical.launchpad.webapp.interfaces import OAuthPermission
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadZopelessLayer,
+    )
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
@@ -27,7 +35,10 @@
     TestHandleStatusMixin,
     )
 from lp.services.job.model.job import Job
-from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.enums import (
+    ArchivePurpose,
+    PackagePublishingStatus,
+    )
 from lp.soyuz.interfaces.binarypackagebuild import (
     IBinaryPackageBuild,
     IBinaryPackageBuildSet,
@@ -39,7 +50,12 @@
 from lp.soyuz.model.buildpackagejob import BuildPackageJob
 from lp.soyuz.model.processor import ProcessorFamilySet
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    api_url,
+    login,
+    logout,
+    TestCaseWithFactory,
+    )
 
 
 class TestBinaryPackageBuild(TestCaseWithFactory):
@@ -170,6 +186,48 @@
         self.assertEquals("Somebody <[e-mail]>",
             self.build.getUploader(MockChanges()))
 
+    def test_can_be_cancelled(self):
+        # For all states that can be cancelled, assert can_be_cancelled
+        # returns True.
+        ok_cases = [
+            BuildStatus.BUILDING,
+            BuildStatus.NEEDSBUILD,
+            ]
+        for status in BuildStatus:
+            if status in ok_cases:
+                self.assertTrue(self.build.can_be_cancelled)
+            else:
+                self.assertFalse(self.build.can_be_cancelled)
+
+    def test_can_be_cancelled_virtuality(self):
+        # Only virtual builds can be cancelled.
+        bq = removeSecurityProxy(self.build.queueBuild())
+        bq.virtualized = True
+        self.assertTrue(self.build.can_be_cancelled)
+        bq.virtualized = False
+        self.assertFalse(self.build.can_be_cancelled)
+
+    def test_cancel_not_in_progress(self):
+        # Testing the cancel() method for a pending build should leave
+        # it in the CANCELLED state.
+        ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        build = self.factory.makeBinaryPackageBuild(archive=ppa)
+        build.queueBuild()
+        build.cancel()
+        self.assertEqual(BuildStatus.CANCELLED, build.status)
+        self.assertIs(None, build.buildqueue_record)
+
+    def test_cancel_in_progress(self):
+        # Testing the cancel() method for a building build should leave
+        # it in the CANCELLING state.
+        ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        build = self.factory.makeBinaryPackageBuild(archive=ppa)
+        bq = build.queueBuild()
+        build.status = BuildStatus.BUILDING
+        build.cancel()
+        self.assertEqual(BuildStatus.CANCELLING, build.status)
+        self.assertEqual(bq, build.buildqueue_record)
+
 
 class TestBuildUpdateDependencies(TestCaseWithFactory):
 
@@ -503,3 +561,59 @@
 class TestHandleStatusForBinaryPackageBuild(
     MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TrialTestCase):
     """IPackageBuild.handleStatus works with binary builds."""
+
+
+class TestBinaryPackageBuildWebservice(TestCaseWithFactory):
+    """Test cases for BinaryPackageBuild on the webservice.
+
+    NB. Note that most tests are currently in
+    lib/lp/soyuz/stories/webservice/xx-builds.txt but unit tests really
+    ought to be here instead.
+    """
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBinaryPackageBuildWebservice, self).setUp()
+        self.ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        self.build = self.factory.makeBinaryPackageBuild(archive=self.ppa)
+        self.webservice = webservice_for_person(
+            self.ppa.owner, permission=OAuthPermission.WRITE_PUBLIC)
+        login(ANONYMOUS)
+
+    def test_can_be_cancelled_is_exported(self):
+        # Check that the can_be_cancelled property is exported.
+        expected = self.build.can_be_cancelled
+        entry_url = api_url(self.build)
+        logout()
+        entry = self.webservice.get(
+            entry_url, api_version='devel').jsonBody()
+        self.assertEqual(expected, entry['can_be_cancelled'])
+
+    def test_cancel_is_exported(self):
+        # Check that the cancel() named op is exported.
+        build_url = api_url(self.build)
+        self.build.queueBuild()
+        logout()
+        entry = self.webservice.get(
+            build_url, api_version='devel').jsonBody()
+        response = self.webservice.named_post(
+            entry['self_link'], 'cancel', api_version='devel')
+        self.assertEqual(200, response.status)
+        entry = self.webservice.get(
+            build_url, api_version='devel').jsonBody()
+        self.assertEqual(BuildStatus.CANCELLED.title, entry['buildstate'])
+
+    def test_cancel_security(self):
+        # Check that unauthorised users cannot call cancel()
+        build_url = api_url(self.build)
+        person = self.factory.makePerson()
+        webservice = webservice_for_person(
+            person, permission=OAuthPermission.WRITE_PUBLIC)
+        logout()
+
+        entry = webservice.get(
+            build_url, api_version='devel').jsonBody()
+        response = webservice.named_post(
+            entry['self_link'], 'cancel', api_version='devel')
+        self.assertEqual(401, response.status)