launchpad-reviewers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

Martin Packman has proposed merging lp:~gz/maas/node_search_view_jujuesque into lp:maas with lp:~gz/maas/node_search_view as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~gz/maas/node_search_view_jujuesque/+merge/128539

As discussed in the prerequisite merge proposal, this branch adapts node search to use the same constraints syntax as the juju command line. It's a little more verbose and fussy, but means copy/pasting between the two will work happily. Note that _parse_constraint now has to raise exceptions rather than just delegating to the constraint resolution to do that, but the view code is unchanged.

Branch also includes a few nice to haves, moves the confusingly duplicated mappings next to each other so it's clearer why both exist, and adds an exception class with a nicer error for when you inevitably tyop a constraint name.
-- 
https://code.launchpad.net/~gz/maas/node_search_view_jujuesque/+merge/128539
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/maas/node_search_view_jujuesque into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-08 11:46:58 +0000
+++ src/maasserver/api.py	2012-10-08 16:21:23 +0000
@@ -157,6 +157,7 @@
     NodeGroupInterface,
     Tag,
     )
+from maasserver.models.node import CONSTRAINTS_MAAS_MAP
 from maasserver.preseed import (
     compose_enlistment_preseed_url,
     compose_preseed_url,
@@ -681,17 +682,6 @@
         return ('nodes_handler', [])
 
 
-# Map the constraint as passed in the request to the name of the constraint in
-# the database. Many of them have the same name
-_constraints_map = {
-    'name': 'hostname',
-    'tags': 'tags',
-    'arch': 'architecture',
-    'cpu_count': 'cpu_count',
-    'mem': 'memory',
-    }
-
-
 def extract_constraints(request_params):
     """Extract a dict of node allocation constraints from http parameters.
 
@@ -701,9 +691,9 @@
     :rtype: :class:`dict`
     """
     constraints = {}
-    for request_name in _constraints_map:
+    for request_name in CONSTRAINTS_MAAS_MAP:
         if request_name in request_params:
-            db_name = _constraints_map[request_name]
+            db_name = CONSTRAINTS_MAAS_MAP[request_name]
             constraints[db_name] = request_params[request_name]
     return constraints
 

=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py	2012-09-26 16:34:58 +0000
+++ src/maasserver/exceptions.py	2012-10-08 16:21:23 +0000
@@ -98,6 +98,16 @@
         return s
 
 
+class NoSuchConstraint(InvalidConstraint):
+    """Node allocation constraint given does not exist."""
+
+    def __init__(self, constraint):
+        super(InvalidConstraint, self).__init__(constraint, None)
+
+    def __str__(self):
+        return "No such '%s' constraint" % self.args[:1]
+
+
 class Redirect(MAASAPIException):
     """Redirect.  The exception message is the target URL."""
     api_error = httplib.FOUND

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-10-05 16:33:37 +0000
+++ src/maasserver/models/node.py	2012-10-08 16:21:23 +0000
@@ -11,6 +11,8 @@
 
 __metaclass__ = type
 __all__ = [
+    "CONSTRAINTS_JUJU_MAP",
+    "CONSTRAINTS_MAAS_MAP",
     "NODE_TRANSITIONS",
     "Node",
     "update_hardware_details",
@@ -77,6 +79,26 @@
     return 'node-%s' % uuid1()
 
 
+# Mapping of constraint names as used by juju to Node field names
+CONSTRAINTS_JUJU_MAP = {
+    'maas-name': 'hostname',
+    'maas-tags': 'tags',
+    'arch': 'architecture',
+    'cpu': 'cpu_count',
+    'mem': 'memory',
+    }
+
+
+# Mapping of constraint names as used by the maas api to Node field names
+CONSTRAINTS_MAAS_MAP = {
+    'name': 'hostname',
+    'tags': 'tags',
+    'arch': 'architecture',
+    'cpu_count': 'cpu_count',
+    'mem': 'memory',
+    }
+
+
 # Information about valid node status transitions.
 # The format is:
 # {

=== modified file 'src/maasserver/tests/test_exceptions.py'
--- src/maasserver/tests/test_exceptions.py	2012-09-26 16:34:58 +0000
+++ src/maasserver/tests/test_exceptions.py	2012-10-08 16:21:23 +0000
@@ -17,6 +17,7 @@
 from maasserver.exceptions import (
     InvalidConstraint,
     MAASAPIBadRequest,
+    NoSuchConstraint,
     Redirect,
     )
 from maasserver.testing import extract_redirect
@@ -57,3 +58,7 @@
         err = InvalidConstraint("limbs", "hundreds", context)
         self.assertEqual(str(err),
             "Invalid 'limbs' constraint 'hundreds': " + str(context))
+
+    def test_NoSuchConstraint_str(self):
+        err = NoSuchConstraint("hue")
+        self.assertEqual(str(err), "No such 'hue' constraint")

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-10-08 16:21:23 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-10-08 16:21:23 +0000
@@ -25,7 +25,10 @@
     NODE_AFTER_COMMISSIONING_ACTION,
     NODE_STATUS,
     )
-from maasserver.exceptions import NoRabbit
+from maasserver.exceptions import (
+    InvalidConstraint,
+    NoRabbit,
+    )
 from maasserver.forms import NodeActionForm
 from maasserver.models import (
     MACAddress,
@@ -369,17 +372,18 @@
         self.assertIn(qs, query_value)
 
     def test_node_list_query_error_on_missing_tag(self):
-        response = self.client.get(reverse('node-list'), {"query": "missing"})
+        response = self.client.get(reverse('node-list'),
+            {"query": "maas-tags=missing"})
         error_string = fromstring(response.content).xpath(
             "string(//div[@id='nodes']//p[@class='form-errors'])")
         self.assertRegexpMatches(error_string, "Invalid .* No such tag")
 
     def test_node_list_query_error_on_unknown_constraint(self):
         response = self.client.get(reverse('node-list'),
-            {"query": "color:red"})
+            {"query": "color=red"})
         error_string = fromstring(response.content).xpath(
             "string(//div[@id='nodes']//p[@class='form-errors'])")
-        self.assertRegexpMatches(error_string, "Invalid .* 'color:red'")
+        self.assertRegexpMatches(error_string, "Invalid .* No such constraint")
 
     def test_node_list_query_selects_subset(self):
         tag = factory.make_tag("shiny")
@@ -390,7 +394,7 @@
         node2.tags = [tag]
         node3.tags = []
         response = self.client.get(reverse('node-list'),
-            {"query": "shiny cpu:2"})
+            {"query": "maas-tags=shiny cpu=2"})
         node2_link = reverse('node-view', args=[node2.system_id])
         document = fromstring(response.content)
         node_links = document.xpath("//div[@id='nodes']/table//a/@href")
@@ -595,59 +599,65 @@
         self.assertEqual({}, constraints)
 
     def test_tag_leading_whitespace(self):
-        constraints = nodes_views._parse_constraints("\ttag")
+        constraints = nodes_views._parse_constraints("\tmaas-tags=tag")
         self.assertEqual({"tags": "tag"}, constraints)
 
     def test_tag_trailing_whitespace(self):
-        constraints = nodes_views._parse_constraints("tag\r\n")
+        constraints = nodes_views._parse_constraints("maas-tags=tag\r\n")
         self.assertEqual({"tags": "tag"}, constraints)
 
     def test_tag_unicode(self):
-        constraints = nodes_views._parse_constraints("\xa7")
+        constraints = nodes_views._parse_constraints("maas-tags=\xa7")
         self.assertEqual({"tags": "\xa7"}, constraints)
 
+    def test_tag_no_value(self):
+        self.assertRaises(InvalidConstraint,
+            nodes_views._parse_constraints, "maas-tags")
+
     def test_cpu(self):
-        constraints = nodes_views._parse_constraints("cpu:1.0")
+        constraints = nodes_views._parse_constraints("cpu=1.0")
         self.assertEqual({"cpu_count": "1.0"}, constraints)
 
     def test_cpu_count(self):
-        constraints = nodes_views._parse_constraints("cpu_count:1")
-        self.assertEqual({"cpu_count": "1"}, constraints)
+        self.assertRaises(InvalidConstraint,
+            nodes_views._parse_constraints, "cpu_count=1.0")
 
     def test_mem(self):
-        constraints = nodes_views._parse_constraints("mem:4096.0")
+        constraints = nodes_views._parse_constraints("mem=4096.0")
         self.assertEqual({"memory": "4096.0"}, constraints)
 
     def test_memory(self):
-        constraints = nodes_views._parse_constraints("memory:4096")
-        self.assertEqual({"memory": "4096"}, constraints)
+        self.assertRaises(InvalidConstraint,
+            nodes_views._parse_constraints, "memory=4096.0")
 
     def test_arch(self):
-        constraints = nodes_views._parse_constraints("arch:armhf/highbank")
+        constraints = nodes_views._parse_constraints("arch=armhf/highbank")
         self.assertEqual({"architecture": "armhf/highbank"}, constraints)
 
+    def test_arch_empty(self):
+        constraints = nodes_views._parse_constraints("arch=")
+        self.assertEqual({}, constraints)
+
     def test_name(self):
-        constraints = nodes_views._parse_constraints("name:node")
+        constraints = nodes_views._parse_constraints("maas-name=node")
         self.assertEqual({"hostname": "node"}, constraints)
 
+    def test_name_any(self):
+        constraints = nodes_views._parse_constraints("maas-name=any")
+        self.assertEqual({}, constraints)
+
     def test_name_unicode(self):
-        constraints = nodes_views._parse_constraints("name:\xa7")
+        constraints = nodes_views._parse_constraints("maas-name=\xa7")
         self.assertEqual({"hostname": "\xa7"}, constraints)
 
     def test_unknown_constraint(self):
-        constraints = nodes_views._parse_constraints("custom:fancy")
-        self.assertEqual({"tags": "custom:fancy"}, constraints)
+        self.assertRaises(InvalidConstraint,
+            nodes_views._parse_constraints, "custom=fancy")
 
     def test_unknown_unicode_constraint(self):
-        constraints = nodes_views._parse_constraints("custom:\xa7")
-        self.assertEqual({"tags": "custom:\xa7"}, constraints)
+        self.assertRaises(InvalidConstraint,
+            nodes_views._parse_constraints, "custom=\xa7")
 
     def test_multiple_tags_and_cpu(self):
-        constraints = nodes_views._parse_constraints("a_tag cpu:2 b_tag")
-        self.assertEqual({"cpu_count": "2", "tags": "a_tag b_tag"},
-            constraints)
-
-    def test_tag_and_cpu_and_mem(self):
-        constraints = nodes_views._parse_constraints("mem:1024 cpu:2 tag")
-        self.assertEqual({"cpu_count": "2", "memory": "1024", "tags": "tag"},
-            constraints)
+        constraints = nodes_views._parse_constraints("maas-tags=a,b cpu=2")
+        self.assertEqual({"cpu_count": "2", "tags": "a,b"}, constraints)

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-10-08 16:21:23 +0000
+++ src/maasserver/views/nodes.py	2012-10-08 16:21:23 +0000
@@ -57,6 +57,7 @@
     MACAddress,
     Node,
     )
+from maasserver.models.node import CONSTRAINTS_JUJU_MAP
 from maasserver.models.node_constraint_filter import constrain_nodes
 from maasserver.preseed import (
     get_enlist_preseed,
@@ -80,33 +81,22 @@
         return {}
 
 
-# Hey look, it's another mapping like the one in maasserver.api
-_non_tag_constraints = {
-    "cpu": "cpu_count",
-    "cpu_count": "cpu_count",
-    "mem": "memory",
-    "memory": "memory",
-    "arch": "architecture",
-    "name": "hostname",
-}
-
-
 def _parse_constraints(query_string):
     """Turn query string from user into constraints dict
 
-    Ideally this would be pretty much the same as the juju constraints, but
-    there are a few things that don't fit nicely across the two models.
+    This is basically the same as the juju constraints, but will differ
+    somewhat in error handling. For instance, juju might reject a negative
+    cpu constraint whereas this lets it through to return zero results.
     """
     constraints = {}
-    tags = []
     for word in query_string.strip().split():
-        parts = word.split(":", 1)
-        if len(parts) == 2 and parts[0] in _non_tag_constraints:
-            constraints[_non_tag_constraints[parts[0]]] = parts[1]
-        else:
-            tags.append(word)
-    if tags:
-        constraints["tags"] = " ".join(tags)
+        parts = word.split("=", 1)
+        if parts[0] not in CONSTRAINTS_JUJU_MAP:
+            raise NoSuchConstraint(parts[0])
+        if len(parts) != 2:
+            raise InvalidConstraint(parts[0], "", "No constraint value given")
+        if parts[1] and parts[1] != "any":
+            constraints[CONSTRAINTS_JUJU_MAP[parts[0]]] = parts[1]
     return constraints