launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1037647 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1037647 in MAAS: "In the API doc, the uri for the nodegroup handler methods are displayed as 'None'."
  https://bugs.launchpad.net/maas/+bug/1037647

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1037647/+merge/120041

This is Raphaël's fix for a small problem he discovered — bug 1037647.  I have nothing to do with it really, and the description in the bug is detailed enough that there's no point in me repeating it here.  Read the bug!

Interestingly, this bug was found by inspecting a hole in our test coverage.  It turns out to be both easy and helpful to run our test suite with coverage analysis.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-1037647/+merge/120041
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-1037647 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-08-15 03:20:44 +0000
+++ src/maasserver/api.py	2012-08-17 02:33:21 +0000
@@ -918,7 +918,7 @@
         return get_object_or_404(NodeGroup, name=name)
 
     @classmethod
-    def resource_uri(cls, nodegroup):
+    def resource_uri(cls, nodegroup=None):
         if nodegroup is None:
             name = 'name'
         else:

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-16 14:10:49 +0000
+++ src/maasserver/tests/test_api.py	2012-08-17 02:33:21 +0000
@@ -2392,12 +2392,13 @@
 class TestNodeGroupsAPI(AnonAPITestCase):
 
     def test_reverse_points_to_nodegroups_api(self):
-        self.assertEqual(self.get_uri('nodegroups/'), reverse('nodegroups'))
+        self.assertEqual(
+            self.get_uri('nodegroups/'), reverse('nodegroups_handler'))
 
     def test_nodegroups_index_lists_nodegroups(self):
         # The nodegroups index lists node groups for the MAAS.
         nodegroup = factory.make_node_group()
-        response = self.client.get(reverse('nodegroups'))
+        response = self.client.get(reverse('nodegroups_handler'))
         self.assertEqual(httplib.OK, response.status_code)
         self.assertIn(nodegroup.name, json.loads(response.content))
 
@@ -2406,7 +2407,7 @@
         self.patch(refresh_worker.refresh_secrets, 'delay', recorder)
         nodegroup = factory.make_node_group()
         response = self.client.post(
-            reverse('nodegroups'), {'op': 'refresh_workers'})
+            reverse('nodegroups_handler'), {'op': 'refresh_workers'})
         self.assertEqual(httplib.OK, response.status_code)
         self.assertIn(
             nodegroup.name, [
@@ -2418,7 +2419,7 @@
         # confirmation.  Anyone can call this method, so it mustn't
         # reveal anything sensitive.
         response = self.client.post(
-            reverse('nodegroups'), {'op': 'refresh_workers'})
+            reverse('nodegroups_handler'), {'op': 'refresh_workers'})
         self.assertEqual(
             (httplib.OK, "Sending worker refresh."),
             (response.status_code, response.content))
@@ -2445,11 +2446,12 @@
         nodegroup = factory.make_node_group()
         self.assertEqual(
             self.get_uri('nodegroups/%s/' % nodegroup.name),
-            reverse('nodegroup', args=[nodegroup.name]))
+            reverse('nodegroup_handler', args=[nodegroup.name]))
 
     def test_GET_returns_node_group(self):
         nodegroup = factory.make_node_group()
-        response = self.client.get(reverse('nodegroup', args=[nodegroup.name]))
+        response = self.client.get(
+            reverse('nodegroup_handler', args=[nodegroup.name]))
         self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(
             nodegroup.name, json.loads(response.content).get('name'))
@@ -2464,7 +2466,7 @@
         factory.make_dhcp_lease(nodegroup=nodegroup)
         client = make_worker_client(nodegroup)
         response = client.post(
-            reverse('nodegroup', args=[nodegroup.name]),
+            reverse('nodegroup_handler', args=[nodegroup.name]),
             {
                 'op': 'update_leases',
                 'leases': json.dumps({}),
@@ -2481,7 +2483,7 @@
         mac = factory.getRandomMACAddress()
         client = make_worker_client(nodegroup)
         response = client.post(
-            reverse('nodegroup', args=[nodegroup.name]),
+            reverse('nodegroup_handler', args=[nodegroup.name]),
             {
                 'op': 'update_leases',
                 'leases': json.dumps({ip: mac}),
@@ -2505,14 +2507,15 @@
 
     def test_nodegroup_requires_authentication(self):
         nodegroup = factory.make_node_group()
-        response = self.client.get(reverse('nodegroup', args=[nodegroup.name]))
+        response = self.client.get(
+            reverse('nodegroup_handler', args=[nodegroup.name]))
         self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
 
     def test_update_leases_works_for_nodegroup_worker(self):
         nodegroup = factory.make_node_group()
         client = make_worker_client(nodegroup)
         response = client.post(
-            reverse('nodegroup', args=[nodegroup.name]),
+            reverse('nodegroup_handler', args=[nodegroup.name]),
             {'op': 'update_leases', 'leases': json.dumps({})})
         self.assertEqual(
             httplib.OK, response.status_code,
@@ -2522,7 +2525,7 @@
         nodegroup = factory.make_node_group()
         self.log_in_as_normal_user()
         response = self.client.post(
-            reverse('nodegroup', args=[nodegroup.name]),
+            reverse('nodegroup_handler', args=[nodegroup.name]),
             {'op': 'update_leases', 'leases': json.dumps({})})
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code,
@@ -2533,7 +2536,7 @@
         about_nodegroup = factory.make_node_group()
         client = make_worker_client(requesting_nodegroup)
         response = client.post(
-            reverse('nodegroup', args=[about_nodegroup.name]),
+            reverse('nodegroup_handler', args=[about_nodegroup.name]),
             {'op': 'update_leases', 'leases': json.dumps({})})
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code,

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2012-08-14 05:56:34 +0000
+++ src/maasserver/urls_api.py	2012-08-17 02:33:21 +0000
@@ -57,7 +57,7 @@
 # API URLs accessible to anonymous users.
 urlpatterns = patterns('',
     url(r'doc/$', api_doc, name='api-doc'),
-    url(r'nodegroups/$', nodegroups_handler, name='nodegroups'),
+    url(r'nodegroups/$', nodegroups_handler, name='nodegroups_handler'),
     url(r'pxeconfig/$', pxeconfig, name='pxeconfig'),
 )
 
@@ -77,7 +77,7 @@
     url(r'nodes/$', nodes_handler, name='nodes_handler'),
     url(
         r'nodegroups/(?P<name>[^/]+)/$',
-        nodegroup_handler, name='nodegroup'),
+        nodegroup_handler, name='nodegroup_handler'),
     url(r'files/$', files_handler, name='files_handler'),
     url(r'account/$', account_handler, name='account_handler'),
 )