Merge lp:~rvb/maas/agent-bug-1239488-list into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Change the API's list() method so that it accepts an optional agent_name parameter to filter the returned nodes.

[Graham Binns (gmb)]

LGTM. One minor tweak for don't-make-me-think-ness:

[1]

33 + factory.make_node(agent_name=factory.make_name('agent-name'))
34 + agent_name = factory.make_name('agent-name')
35 + node = factory.make_node(agent_name=agent_name)

I'm assuming that this first factory.make_node() is to create a node that won't show up in the list. That's fine, but let's do something like:

    factory.make_node(agent_name=factory.make_name('this-wont-be-listed'))

Otherwise dimwits like your reviewer will get confused by the two calls to factory.make_name('agent-name').

review: Approve

[Raphaël Badin (rvb)]

> LGTM. One minor tweak for don't-make-me-think-ness:
>
> [1]
>
> 33 + factory.make_node(agent_name=factory.make_name('agent-name'))
> 34 + agent_name = factory.make_name('agent-name')
> 35 + node = factory.make_node(agent_name=agent_name)
>
> I'm assuming that this first factory.make_node() is to create a node that
> won't show up in the list. That's fine, but let's do something like:
>
> factory.make_node(agent_name=factory.make_name('this-wont-be-listed'))
>
> Otherwise dimwits like your reviewer will get confused by the two calls to
> factory.make_name('agent-name').

You've got a valid point, but I think it's better to name the created node to indicate it's purpose rather than using the agent_name to denote that so I've changed the code to:

45 + non_listed_node = factory.make_node(
46 + agent_name=factory.make_name('agent_name'))
47 + ignore_unused(non_listed_node)

Thanks for the review!

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-10-15 11:53:38 +0000
+++ src/maasserver/api.py	2013-10-15 11:53:38 +0000
@@ -736,6 +736,9 @@
         :param id: An optional list of system ids.  Only nodes with
             matching system ids will be returned.
         :type id: iterable
+        :param agent_name: An optional agent name.  Only nodes with
+            matching agent names will be returned.
+        :type agent_name: unicode
         """
         # Get filters from request.
         match_ids = get_optional_list(request.GET, 'id')
@@ -751,6 +754,9 @@
             request.user, NODE_PERMISSION.VIEW, ids=match_ids)
         if match_macs is not None:
             nodes = nodes.filter(macaddress__mac_address__in=match_macs)
+        match_agent_name = request.GET.get('agent_name', None)
+        if match_agent_name is not None:
+            nodes = nodes.filter(agent_name=match_agent_name)
         # Prefetch related macaddresses, tags and nodegroups (plus
         # related interfaces).
         nodes = nodes.prefetch_related('macaddress_set__node')

=== modified file 'src/maasserver/tests/test_api_nodes.py'
--- src/maasserver/tests/test_api_nodes.py	2013-10-15 11:53:38 +0000
+++ src/maasserver/tests/test_api_nodes.py	2013-10-15 11:53:38 +0000
@@ -41,7 +41,10 @@
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from maasserver.utils import map_enum
+from maasserver.utils import (
+    ignore_unused,
+    map_enum,
+    )
 from maasserver.utils.orm import get_one
 from testtools.matchers import (
     Contains,
@@ -306,6 +309,44 @@
             )
         self.assertThat(observed, MatchesListwise(expected))
 
+    def test_GET_list_with_agent_name_filters_by_agent_name(self):
+        non_listed_node = factory.make_node(
+            agent_name=factory.make_name('agent_name'))
+        ignore_unused(non_listed_node)
+        agent_name = factory.make_name('agent-name')
+        node = factory.make_node(agent_name=agent_name)
+        response = self.client.get(self.get_uri('nodes/'), {
+            'op': 'list',
+            'agent_name': agent_name,
+            })
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertSequenceEqual(
+            [node.system_id], extract_system_ids(parsed_result))
+
+    def test_GET_list_with_agent_name_filters_with_empty_string(self):
+        factory.make_node(agent_name=factory.make_name('agent-name'))
+        node = factory.make_node(agent_name='')
+        response = self.client.get(self.get_uri('nodes/'), {
+            'op': 'list',
+            'agent_name': '',
+            })
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertSequenceEqual(
+            [node.system_id], extract_system_ids(parsed_result))
+
+    def test_GET_list_without_agent_name_does_not_filter(self):
+        nodes = [
+            factory.make_node(agent_name=factory.make_name('agent-name'))
+            for i in range(3)]
+        response = self.client.get(self.get_uri('nodes/'), {'op': 'list'})
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertSequenceEqual(
+            [node.system_id for node in nodes],
+            extract_system_ids(parsed_result))
+
     def test_GET_list_allocated_returns_only_allocated_with_user_token(self):
         # If the user's allocated nodes have different session tokens,
         # list_allocated should only return the nodes that have the