Code review comment for lp:~racb/maas/subarch-model

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good! A few comments, but they're all very minor.

[1]

+        architecture = factory.getRandomChoice(
+            filter(lambda choice: choice[0].endswith('/generic'),
+                   ARCHITECTURE_CHOICES))

On the whole we try to avoid filter and map, because comprehensions
are normally much easier to read, and because their behaviour changes
in Python 3. I think the above would clearer as:

        architecture = factory.getRandomChoice(
            [choice for choice in ARCHITECTURE_CHOICES
             if choice[0].endswith('/generic')])

[2]

+        parsed_result = json.loads(response.content)
+
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertIn('application/json', response['Content-Type'])

Don't parse the result until after the response has been checked; it
might fail and then we know less about the failure. How about:

        self.assertEqual(
            (httplib.OK, 'application/json'),
            (response.status_code, response['Content-Type']),
            response.content)
        parsed_result = json.loads(response.content)

If the response is not OK or the content-type is not JSON, this will
blow up with the mismatch and the response content as detail.

[3]

+        self.assertIn('text/plain; charset=utf-8', response['Content-Type'])

There's assertStartsWith too, if you just want to check that it's
text/plain.

[4]

+        self.assertEqual("Subarchitecture cannot be specified twice",

Sentences end in a full-stop... unless the convention elsewhere in the
codebase to not do that. Very minor point, but it makes a difference
to the overall polish.

[5]

+        self.assertEqual(expected_arch[0], params_out["arch"])
+        self.assertEqual(expected_arch[1], params_out["subarch"])

Failures would be more informative if this were done in a single
assertion:

        observed_arch = params_out["arch"], params_out["subarch"]
        self.assertEqual(expected_arch, observed_arch)

[6]

         node_template_name = "%s_%s_%s_%s" % (
-            prefix, node.architecture, release, node.hostname)
+            prefix, node.architecture.replace('/', '_'),
+            release, node.hostname)

Because the components are delimited by _, perhaps replace / in the
architecture with - or : instead?

review: Approve

« Back to merge proposal