Merge lp:~jtv/maas/api-list-networks into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

"List networks" API call.

In doing this I noticed a change in our API style: where we used to write "list" operations for the "plural" resources, unless we need to parameterise the listing, we now just implement GET on the resource. So to list all nodes, you call the nodes handler's list() method, but to list all zones, you just GET ".../zones/". I followed the new style here, but I'm not sure the CLI can deal with this. Maybe we ought to have both for all resources.

Jeroen

[Raphaël Badin (rvb)]

[0]

50 === added file 'src/maasserver/tests/test_api_networks.py'
[...]
83 + def test_list_returns_network(self):

The name of this test is strange. One would expect test_list_returns_network*s*

[1]

50 === added file 'src/maasserver/tests/test_api_networks.py'
[...]
83 + def test_list_returns_network(self):

I put a break point in this test and look:

(Pdb) parsed_result
[{u'description': u'tjBXqFQ7vK', u'ip': u'217.187.192.0', u'_state': u'<django.db.models.base.ModelState object at 0x962acd0>', u'netmask': u'255.255.192.0', u'vlan_tag': None, u'id': 1, u'name': u'6THuCI'}]

Django adds clutter to the objects (I'm talking about the '_state' field) so we need to explicitly list the fields that we want to return.

[2]

50 === added file 'src/maasserver/tests/test_api_networks.py'
[...]
93 + (
94 + original_network.name,
95 + original_network.ip,
96 + original_network.netmask,
97 + original_network.vlan_tag,
98 + original_network.description,
99 + ),
100 + (
101 + returned_network['name'],
102 + returned_network['ip'],
103 + returned_network['netmask'],
104 + returned_network['vlan_tag'],
105 + returned_network['description'],
106 + ))

Couldn't this benefit from self.assertAttributes?

[3]

50 === added file 'src/maasserver/tests/test_api_networks.py'
[...]
93 + (
94 + original_network.name,
95 + original_network.ip,
96 + original_network.netmask,
97 + original_network.vlan_tag,
98 + original_network.description,
99 + ),
100 + (
101 + returned_network['name'],
102 + returned_network['ip'],
103 + returned_network['netmask'],
104 + returned_network['vlan_tag'],
105 + returned_network['description'],
106 + ))

There is a bug: we should also have a 'resource_uri' field.

[Raphaël Badin (rvb)]

No further remarks :).

[1] and [3] are, I think, blocker but feel free to fix that in follow-up branches.

I see you've added the 'resource_uri' field in https://code.launchpad.net/~jtv/maas/api-add-edit-network/+merge/205539 so it's just a matter of adding a test to fix [3].

review: Approve

[Jeroen T. Vermeulen (jtv)]

> [0]
>
> 50 === added file 'src/maasserver/tests/test_api_networks.py'
> [...]
> 83 + def test_list_returns_network(self):
>
> The name of this test is strange. One would expect
> test_list_returns_network*s*

Except in this case it's only one.

> [1]
>
> 50 === added file 'src/maasserver/tests/test_api_networks.py'
> [...]
> 83 + def test_list_returns_network(self):
>
> I put a break point in this test and look:
>
> (Pdb) parsed_result
> [{u'description': u'tjBXqFQ7vK', u'ip': u'217.187.192.0', u'_state':
> u'<django.db.models.base.ModelState object at 0x962acd0>', u'netmask':
> u'255.255.192.0', u'vlan_tag': None, u'id': 1, u'name': u'6THuCI'}]
>
> Django adds clutter to the objects (I'm talking about the '_state' field) so
> we need to explicitly list the fields that we want to return.

That goes in NetworkHandler.fields I believe, which is in one of the later branches in this series. I'll add it there.

> [2]
>
> 50 === added file 'src/maasserver/tests/test_api_networks.py'
> [...]
> 93 + (
> 94 + original_network.name,
> 95 + original_network.ip,
> 96 + original_network.netmask,
> 97 + original_network.vlan_tag,
> 98 + original_network.description,
> 99 + ),
> 100 + (
> 101 + returned_network['name'],
> 102 + returned_network['ip'],
> 103 + returned_network['netmask'],
> 104 + returned_network['vlan_tag'],
> 105 + returned_network['description'],
> 106 + ))
>
> Couldn't this benefit from self.assertAttributes?

The same words went through my head, believe me! The problem is that in this case, the object has the expected values and the dict has the tested values.

> [3]
>
> 50 === added file 'src/maasserver/tests/test_api_networks.py'
> [...]
> 93 + (
> 94 + original_network.name,
> 95 + original_network.ip,
> 96 + original_network.netmask,
> 97 + original_network.vlan_tag,
> 98 + original_network.description,
> 99 + ),
> 100 + (
> 101 + returned_network['name'],
> 102 + returned_network['ip'],
> 103 + returned_network['netmask'],
> 104 + returned_network['vlan_tag'],
> 105 + returned_network['description'],
> 106 + ))
>
> There is a bug: we should also have a 'resource_uri' field.

Yeah, I added that in one of the other branches.

[Raphaël Badin (rvb)]

> > [0]
> >
> > 50 === added file 'src/maasserver/tests/test_api_networks.py'
> > [...]
> > 83 + def test_list_returns_network(self):
> >
> > The name of this test is strange. One would expect
> > test_list_returns_network*s*
>
> Except in this case it's only one.

Well, I'd say it's a list of networks… with only one network. For the casual reader, test_list_returns_network looks weird.

[Jeroen T. Vermeulen (jtv)]

OK, I renamed the test.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-02-07 17:14:08 +0000
+++ src/maasserver/api.py	2014-02-10 11:22:04 +0000
@@ -69,6 +69,7 @@
     "FilesHandler",
     "get_oauth_token",
     "MaasHandler",
+    "NetworksHandler",
     "NodeGroupHandler",
     "NodeGroupsHandler",
     "NodeGroupInterfaceHandler",
@@ -193,6 +194,7 @@
     DHCPLease,
     FileStorage,
     MACAddress,
+    Network,
     Node,
     NodeGroup,
     NodeGroupInterface,
@@ -2590,11 +2592,10 @@
 class ZoneHandler(OperationsHandler):
     """Manage a physical zone.
 
-    Any node may optionally be assigned to a physical zone, or "zone" for
-    short.  The meaning of a physical zone is up to you: it could identify
-    e.g. a server rack, a network, or a data centre.  Users can then allocate
-    nodes from specific physical zones, to suit their redundancy or
-    performance requirements.
+    Any node is in a physical zone, or "zone" for short.  The meaning of a
+    physical zone is up to you: it could identify e.g. a server rack, a
+    network, or a data centre.  Users can then allocate nodes from specific
+    physical zones, to suit their redundancy or performance requirements.
 
     This functionality is only available to administrators.  Other users can
     view physical zones, but not modify them.
@@ -2665,3 +2666,12 @@
         Get a listing of all the physical zones.
         """
         return Zone.objects.all().order_by('name')
+
+
+class NetworksHandler(OperationsHandler):
+    """API for networks."""
+    create = update = delete = None
+
+    def read(self, request):
+        """List networks."""
+        return Network.objects.all().order_by('name')

=== added file 'src/maasserver/tests/test_api_networks.py'
--- src/maasserver/tests/test_api_networks.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_api_networks.py	2014-02-10 11:22:04 +0000
@@ -0,0 +1,70 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for networks API."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+import httplib
+import json
+
+from django.core.urlresolvers import reverse
+from maasserver.testing.api import APITestCase
+from maasserver.testing.factory import factory
+
+
+class TestNetworksAPI(APITestCase):
+
+    def test_handler_path(self):
+        self.assertEqual('/api/1.0/networks/', reverse('networks_handler'))
+
+    def test_list_returns_networks(self):
+        original_network = factory.make_network()
+
+        response = self.client.get(reverse('networks_handler'))
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+
+        parsed_result = json.loads(response.content)
+        self.assertEqual(1, len(parsed_result))
+        [returned_network] = parsed_result
+        self.assertEqual(
+            (
+                original_network.name,
+                original_network.ip,
+                original_network.netmask,
+                original_network.vlan_tag,
+                original_network.description,
+            ),
+            (
+                returned_network['name'],
+                returned_network['ip'],
+                returned_network['netmask'],
+                returned_network['vlan_tag'],
+                returned_network['description'],
+            ))
+
+    def test_list_returns_empty_if_no_networks(self):
+        response = self.client.get(reverse('networks_handler'))
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertEqual([], json.loads(response.content))
+
+    def test_list_sorts_by_name(self):
+        original_names = [factory.make_name('net').lower() for _ in range(3)]
+        for name in original_names:
+            factory.make_network(name=name)
+
+        response = self.client.get(reverse('networks_handler'))
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+
+        self.assertEqual(
+            sorted(original_names),
+            [network['name'] for network in json.loads(response.content)])

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2013-12-13 11:43:26 +0000
+++ src/maasserver/urls_api.py	2014-02-10 11:22:04 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """URL API routing configuration."""
@@ -29,6 +29,7 @@
     FileHandler,
     FilesHandler,
     MaasHandler,
+    NetworksHandler,
     NodeGroupHandler,
     NodeGroupInterfaceHandler,
     NodeGroupInterfacesHandler,
@@ -57,6 +58,7 @@
 account_handler = RestrictedResource(AccountHandler, authentication=api_auth)
 files_handler = RestrictedResource(FilesHandler, authentication=api_auth)
 file_handler = RestrictedResource(FileHandler, authentication=api_auth)
+networks_handler = RestrictedResource(NetworksHandler, authentication=api_auth)
 node_handler = RestrictedResource(NodeHandler, authentication=api_auth)
 nodes_handler = RestrictedResource(NodesHandler, authentication=api_auth)
 node_mac_handler = RestrictedResource(NodeMacHandler, authentication=api_auth)
@@ -126,6 +128,7 @@
         nodegroupinterfaces_handler, name='nodegroupinterfaces_handler'),
     url(r'^nodegroups/(?P<uuid>[^/]+)/interfaces/(?P<interface>[^/]+)/$',
         nodegroupinterface_handler, name='nodegroupinterface_handler'),
+    url(r'^networks/$', networks_handler, name='networks_handler'),
     url(r'^files/$', files_handler, name='files_handler'),
     url(r'^files/(?P<filename>.+)/$', file_handler, name='file_handler'),
     url(r'^account/$', account_handler, name='account_handler'),