launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/check-commissioning-bug-1084292 into lp:maas.

Commit message:
Fix the check_commissioning API call: make it available to the logged-in users only and return the changed nodes (which is in accordance with what the docs already say).

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1084292 in MAAS: "maas-cli maas nodes check-commissioning returns a Unrecognised signature"
  https://bugs.launchpad.net/maas/+bug/1084292

For more details, see:
https://code.launchpad.net/~rvb/maas/check-commissioning-bug-1084292/+merge/137526
-- 
https://code.launchpad.net/~rvb/maas/check-commissioning-bug-1084292/+merge/137526
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/check-commissioning-bug-1084292 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-29 11:30:37 +0000
+++ src/maasserver/api.py	2012-12-03 10:47:22 +0000
@@ -458,21 +458,6 @@
         """Accept a node's enlistment: not allowed to anonymous users."""
         raise Unauthorized("You must be logged in to accept nodes.")
 
-    @operation(idempotent=False)
-    def check_commissioning(self, request):
-        """Check all commissioning nodes to see if they are taking too long.
-
-        Anything that has been commissioning for longer than
-        settings.COMMISSIONING_TIMEOUT is moved into the FAILED_TESTS status.
-        """
-        interval = timedelta(minutes=settings.COMMISSIONING_TIMEOUT)
-        cutoff = datetime.now() - interval
-        query = Node.objects.filter(
-            status=NODE_STATUS.COMMISSIONING, updated__lte=cutoff)
-        query.update(status=NODE_STATUS.FAILED_TESTS)
-        # Note that Django doesn't call save() on updated nodes here,
-        # but I don't think anything requires its effects anyway.
-
     @classmethod
     def resource_uri(cls, *args, **kwargs):
         return ('nodes_handler', [])
@@ -591,6 +576,23 @@
         return filter(None, nodes)
 
     @operation(idempotent=False)
+    def check_commissioning(self, request):
+        """Check all commissioning nodes to see if they are taking too long.
+
+        Anything that has been commissioning for longer than
+        settings.COMMISSIONING_TIMEOUT is moved into the FAILED_TESTS status.
+        """
+        interval = timedelta(minutes=settings.COMMISSIONING_TIMEOUT)
+        cutoff = datetime.now() - interval
+        query = Node.objects.filter(
+            status=NODE_STATUS.COMMISSIONING, updated__lte=cutoff)
+        results = list(query)
+        query.update(status=NODE_STATUS.FAILED_TESTS)
+        # Note that Django doesn't call save() on updated nodes here,
+        # but I don't think anything requires its effects anyway.
+        return results
+
+    @operation(idempotent=False)
     def release(self, request):
         """Release multiple nodes.
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-28 15:37:34 +0000
+++ src/maasserver/tests/test_api.py	2012-12-03 10:47:22 +0000
@@ -3284,24 +3284,28 @@
             (response.status_code, response.content))
 
 
-class TestAnonymousCommissioningTimeout(APIv10TestMixin, TestCase):
+class TestCommissioningTimeout(APIv10TestMixin, LoggedInTestCase):
     """Testing of commissioning timeout API."""
 
     def test_check_with_no_action(self):
         node = factory.make_node(status=NODE_STATUS.READY)
-        self.client.post(
+        response = self.client.post(
             self.get_uri('nodes/'), {'op': 'check_commissioning'})
         # Anything that's not commissioning should be ignored.
         node = reload_object(node)
-        self.assertEqual(NODE_STATUS.READY, node.status)
+        self.assertEqual(
+            (httplib.OK, NODE_STATUS.READY),
+            (response.status_code, node.status))
 
     def test_check_with_commissioning_but_not_expired_node(self):
         node = factory.make_node(
             status=NODE_STATUS.COMMISSIONING)
-        self.client.post(
+        response = self.client.post(
             self.get_uri('nodes/'), {'op': 'check_commissioning'})
         node = reload_object(node)
-        self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
+        self.assertEqual(
+            (httplib.OK, NODE_STATUS.COMMISSIONING),
+            (response.status_code, node.status))
 
     def test_check_with_commissioning_and_expired_node(self):
         # Have an interval 1 second longer than the timeout.
@@ -3311,10 +3315,15 @@
             status=NODE_STATUS.COMMISSIONING, created=datetime.now(),
             updated=updated_at)
 
-        self.client.post(
+        response = self.client.post(
             self.get_uri('nodes/'), {'op': 'check_commissioning'})
-        node = reload_object(node)
-        self.assertEqual(NODE_STATUS.FAILED_TESTS, node.status)
+        self.assertEqual(
+            (httplib.OK, NODE_STATUS.FAILED_TESTS, [node.system_id]),
+            (
+                response.status_code,
+                reload_object(node).status,
+                [node['system_id'] for node in json.loads(response.content)],
+            ))
 
 
 class TestPXEConfigAPI(AnonAPITestCase):