launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/builder-protocol-ui-api into lp:launchpad.

Commit message:
Include Builder.vm_reset_protocol on add/edit forms, and make Builder.clean_status writable through the webservice.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/builder-protocol-ui-api/+merge/223879

Include Builder.vm_reset_protocol on add/edit forms, and make Builder.clean_status writable through the webservice.
-- 
https://code.launchpad.net/~wgrant/launchpad/builder-protocol-ui-api/+merge/223879
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/builder-protocol-ui-api into lp:launchpad.

=== modified file 'lib/lp/buildmaster/browser/builder.py'
--- lib/lp/buildmaster/browser/builder.py	2014-05-07 10:01:04 +0000
+++ lib/lp/buildmaster/browser/builder.py	2014-06-20 09:33:19 +0000
@@ -331,7 +331,7 @@
 
     field_names = [
         'name', 'title', 'processors', 'url', 'active', 'virtualized',
-        'vm_host', 'owner'
+        'vm_host', 'vm_reset_protocol', 'owner'
         ]
     custom_widget('owner', HiddenUserWidget)
     custom_widget('url', TextWidget, displayWidth=30)
@@ -350,6 +350,7 @@
             active=data.get('active'),
             virtualized=data.get('virtualized'),
             vm_host=data.get('vm_host'),
+            vm_reset_protocol=data.get('vm_reset_protocol'),
             )
         notify(ObjectCreatedEvent(builder))
         self.next_url = canonical_url(builder)
@@ -372,7 +373,8 @@
 
     field_names = [
         'name', 'title', 'processors', 'url', 'manual', 'owner',
-        'virtualized', 'builderok', 'failnotes', 'vm_host', 'active',
+        'virtualized', 'builderok', 'failnotes', 'vm_host',
+        'vm_reset_protocol', 'active',
         ]
     custom_widget('processors', LabeledMultiCheckBoxWidget)
 

=== modified file 'lib/lp/buildmaster/configure.zcml'
--- lib/lp/buildmaster/configure.zcml	2014-04-23 10:50:06 +0000
+++ lib/lp/buildmaster/configure.zcml	2014-06-20 09:33:19 +0000
@@ -16,10 +16,11 @@
     <class
         class="lp.buildmaster.model.builder.Builder">
         <allow
-            interface="lp.buildmaster.interfaces.builder.IBuilder"/>
+            interface="lp.buildmaster.interfaces.builder.IBuilderView"/>
         <require
             permission="launchpad.Edit"
-            set_schema="lp.buildmaster.interfaces.builder.IBuilder"/>
+            interface="lp.buildmaster.interfaces.builder.IBuilderEdit"
+            set_schema="lp.buildmaster.interfaces.builder.IBuilderView"/>
     </class>
 
 

=== modified file 'lib/lp/buildmaster/interfaces/builder.py'
--- lib/lp/buildmaster/interfaces/builder.py	2014-06-17 12:59:03 +0000
+++ lib/lp/buildmaster/interfaces/builder.py	2014-06-20 09:33:19 +0000
@@ -23,7 +23,9 @@
     export_as_webservice_entry,
     export_factory_operation,
     export_read_operation,
+    export_write_operation,
     exported,
+    mutator_for,
     operation_for_version,
     operation_parameters,
     operation_returns_collection_of,
@@ -34,6 +36,7 @@
     Reference,
     ReferenceChoice,
     )
+from lazr.restful.interface import copy_field
 from zope.interface import (
     Attribute,
     Interface,
@@ -95,19 +98,7 @@
     """The build slave has suffered an error and cannot be used."""
 
 
-class IBuilder(IHasBuildRecords, IHasOwner):
-    """Build-slave information and state.
-
-    Builder instance represents a single builder slave machine within the
-    Launchpad Auto Build System. It should specify a 'processor' on which the
-    machine is based and is able to build packages for; a URL, by which the
-    machine is accessed through an XML-RPC interface; name, title for entity
-    identification and browsing purposes; an LP-like owner which has
-    unrestricted access to the instance; the build slave machine status
-    representation, including the field/properties: virtualized, builderok,
-    status, failnotes and currentjob.
-    """
-    export_as_webservice_entry()
+class IBuilderView(IHasBuildRecords, IHasOwner):
 
     id = Attribute("Builder identifier")
 
@@ -169,10 +160,15 @@
         description=_('The reason for a builder not being ok')))
 
     vm_host = exported(TextLine(
-        title=_('Virtual Machine Host'), required=False,
+        title=_('VM host'), required=False,
         description=_('The machine hostname hosting the virtual '
                       'buildd-slave, e.g.: foobar-host.ppa')))
 
+    vm_reset_protocol = exported(Choice(
+        title=_("VM reset protocol"), vocabulary=BuilderResetProtocol,
+        readonly=False, required=False,
+        description=_("The protocol version for resetting the VM.")))
+
     active = exported(Bool(
         title=_('Publicly Visible'), required=False, default=True,
         description=_('Whether or not to present the builder publicly.')))
@@ -189,20 +185,14 @@
 
     clean_status = exported(Choice(
         title=_("Clean status"), vocabulary=BuilderCleanStatus, readonly=True,
-        description=_("The readiness of the slave to take a job.")))
+        description=_(
+            "The readiness of the slave to take a job. Only internal build "
+            "infrastructure bots need to or should write to this.")))
 
     date_clean_status_changed = exported(Datetime(
         title=_("Date clean status changed"), readonly=True,
         description=_("The date the builder's clean status last changed.")))
 
-    vm_reset_protocol = exported(Choice(
-        title=_("VM reset protocol"), vocabulary=BuilderResetProtocol,
-        readonly=False,
-        description=_("The protocol version for resetting the VM.")))
-
-    def setCleanStatus(status):
-        """Update the clean status."""
-
     def gotFailure():
         """Increment failure_count on the builder."""
 
@@ -235,6 +225,31 @@
         """
 
 
+class IBuilderEdit(Interface):
+
+    @mutator_for(IBuilderView['clean_status'])
+    @operation_parameters(status=copy_field(IBuilderView['clean_status']))
+    @export_write_operation()
+    @operation_for_version('devel')
+    def setCleanStatus(status):
+        """Update the clean status."""
+
+
+class IBuilder(IBuilderEdit, IBuilderView):
+    """Build-slave information and state.
+
+    Builder instance represents a single builder slave machine within the
+    Launchpad Auto Build System. It should specify a 'processor' on which the
+    machine is based and is able to build packages for; a URL, by which the
+    machine is accessed through an XML-RPC interface; name, title for entity
+    identification and browsing purposes; an LP-like owner which has
+    unrestricted access to the instance; the build slave machine status
+    representation, including the field/properties: virtualized, builderok,
+    status, failnotes and currentjob.
+    """
+    export_as_webservice_entry()
+
+
 class IBuilderSetAdmin(Interface):
 
     @call_with(owner=REQUEST_USER)

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2014-06-17 12:59:51 +0000
+++ lib/lp/buildmaster/model/builder.py	2014-06-20 09:33:19 +0000
@@ -344,11 +344,13 @@
         return self.getByName(name)
 
     def new(self, processors, url, name, title, owner, active=True,
-            virtualized=False, vm_host=None, manual=True):
+            virtualized=False, vm_host=None, vm_reset_protocol=None,
+            manual=True):
         """See IBuilderSet."""
         return Builder(processors=processors, url=url, name=name, title=title,
                        owner=owner, active=active, virtualized=virtualized,
-                       vm_host=vm_host, _builderok=True, manual=manual)
+                       vm_host=vm_host, vm_reset_protocol=vm_reset_protocol,
+                       _builderok=True, manual=manual)
 
     def get(self, builder_id):
         """See IBuilderSet."""

=== modified file 'lib/lp/buildmaster/tests/test_webservice.py'
--- lib/lp/buildmaster/tests/test_webservice.py	2014-04-23 10:50:06 +0000
+++ lib/lp/buildmaster/tests/test_webservice.py	2014-06-20 09:33:19 +0000
@@ -5,6 +5,8 @@
 
 __metaclass__ = type
 
+from json import dumps
+
 from zope.component import getUtility
 
 from lp.registry.interfaces.person import IPersonSet
@@ -86,6 +88,30 @@
         super(TestBuilderEntry, self).setUp()
         self.webservice = LaunchpadWebServiceCaller()
 
+    def test_security(self):
+        # Attributes can only be set by buildd admins.
+        builder = self.factory.makeBuilder()
+        user = self.factory.makePerson()
+        user_webservice = webservice_for_person(
+            user, permission=OAuthPermission.WRITE_PUBLIC)
+        patch = dumps({'clean_status': 'Cleaning'})
+        logout()
+
+        # A normal user is unauthorized.
+        response = user_webservice.patch(
+            api_url(builder), 'application/json', patch, api_version='devel')
+        self.assertEqual(401, response.status)
+
+        # But a buildd admin can set the attribute.
+        with admin_logged_in():
+            buildd_admins = getUtility(IPersonSet).getByName(
+                'launchpad-buildd-admins')
+            buildd_admins.addMember(user, buildd_admins.teamowner)
+        response = user_webservice.patch(
+            api_url(builder), 'application/json', patch, api_version='devel')
+        self.assertEqual(209, response.status)
+        self.assertEqual('Cleaning', response.jsonBody()['clean_status'])
+
     def test_exports_processor(self):
         processor = self.factory.makeProcessor('s1')
         builder = self.factory.makeBuilder(processors=[processor])

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-buildfarm-index.txt'
--- lib/lp/soyuz/stories/soyuz/xx-buildfarm-index.txt	2013-12-03 05:18:41 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-buildfarm-index.txt	2014-06-20 09:33:19 +0000
@@ -159,12 +159,14 @@
 
     >>> print admin_browser.getControl('Virtualized').selected
     False
-    >>> admin_browser.getControl('Virtual Machine Host').value
+    >>> admin_browser.getControl('VM host').value
     ''
+    >>> admin_browser.getControl('VM reset protocol').value
+    ['']
 
     >>> admin_browser.getControl('Virtualized').selected = True
-    >>> admin_browser.getControl(
-    ...     'Virtual Machine Host').value = 'tubaina-host.ppa'
+    >>> admin_browser.getControl('VM host').value = 'tubaina-host.ppa'
+    >>> admin_browser.getControl('VM reset protocol').value = ['PROTO_1_1']
 
 Once the form is submitted the users will be redirected to the
 just-created builder page.
@@ -176,3 +178,9 @@
     Tubaina : Build Farm
     >>> 'amd64 and hppa' in admin_browser.contents
     True
+
+    >>> admin_browser.getLink("Change details").click()
+    >>> admin_browser.getControl('VM host').value
+    'tubaina-host.ppa'
+    >>> admin_browser.getControl('VM reset protocol').value
+    ['PROTO_1_1']