Code review comment for lp://qastaging/~rvb/maas/maas-node-api

Looks good :) I have a lot of comments, but they're all minor/trivial,
and you may want to disagree with many of them.

[1]

+ message = []

s/message/messages/

Plural for a list?

[2]

+ for k, v in error.message_dict.items():

s/items/iteritems/

[3]

+ if type(v) is list:

if isinstance(v, list):

[4]

+ return get_object_or_404(Node, system_id=system_id)

Django is a massive layer violation isn't it? ;)

[5]

+ model = Node
...
+ def read(self, request, system_id=None):
+ if system_id is not None:
+ return get_object_or_404(Node, system_id=system_id)
+ else:
+ return Node.objects.all().order_by('id')

Does it make sense to s/Node/self.model/ above (and elsewhere)?

[6]

+ node = Node(status = 'NEW', **dict(request.data.items()))

Pyflakes ought to be moaning about the spaces around the =, bit it's
curiously silent. Ah, maybe it's pep8 that ought to be moaning...

[7]

+ if 'status' in request.data:
+ return bad_request('Cannot set the status for a node.')

This worries me. Do we need to check every bit of data coming in by
hand? I don't want to have to do that, otherwise we're going to have
to check handlers every time we update the model.

[8]

How do we layer authorization onto this?

[9]

- hostname = models.CharField(max_length=255)
+ hostname = models.CharField(max_length=255, null=True, default=None)

Is there a min_length too? A hostname of "" is as good as null, so
it's probably better to have just one or the other.

[10]

+ mac = MACAddress(mac_address=mac_address, node=self)
+ mac.full_clean()
+ mac.save()

Does mac.save() not imply full_clean()? Seems like a common pattern,
and it's strange that Django doesn't just do it.

[11]

+ def addMACAddress(self, mac_address):
...
+ def removeMACAddress(self, mac_address):

We have a chance to adopt PEP8 naming (i.e. add_mac_address,
remove_mac_address). Perhaps we ought to.

[12]

=== added file 'src/maasserver/tests/factory.py'

This should probably like in testing/factory.py.

[13]

+class Factory():
...
+ def createNode(self, hostname=None, set_hostname=False, status=None,

Should we maintain the Launchpad naming here? i.e. makeSomething (or
make_something if we're going PEP8). createSomething feels like proper
API, if you know what I mean.

[14]

+ self.assertEqual(2, len(parsed_result))
+ self.assertEqual(node1.hostname, parsed_result[0]['hostname'])
+ self.assertEqual(node1.system_id, parsed_result[0]['system_id'])
+ self.assertEqual(node2.hostname, parsed_result[1]['hostname'])
+ self.assertEqual(node2.system_id, parsed_result[1]['system_id'])

This might be clearer and simpler as:

        expected = [
            {"hostname": node1.hostname, "system_id": node1.system_id},
            {"hostname": node2.hostname, "system_id": node2.system_id},
            ]

        self.assertEqual(expected, parsed_result)

[15]

+ """
+ The api allows to fetch the list of Nodes.

s/allows to fetch/allows for fetching/

(Elsewhere too.)

[16]

+ def test_node_DELETE(self):
...
+ self.assertEqual(
+ 0,
+ Node.objects.filter(system_id = system_id).count())

The following might be clearer:

+ self.assertEqual(
+ [], list(Node.objects.filter(system_id=system_id)))

(Is the list() needed?)

[17]

+ def setUp(self):
+ self.node = factory.createNode()
+ self.mac1 = self.node.addMACAddress('aa:bb:cc:dd:ee:ff')
+ self.mac2 = self.node.addMACAddress('22:bb:cc:dd:aa:ff')

Don't forget to up-call.

[18]

+ self.assertEqual(
+ nb_macs + 1,
+ MACAddress.objects.filter(node = self.node).count())

Like above, show the MACs instead of just the count.

[19]

+ def xtest_macs_DELETE_mac(self):

s/xtest/test/ ?

[20]

+ When deleting a MAC Address, the api returns a 'Bad Request' (400)
+ error the provided MAC Address is not valid.

s/error/error if/

[21]

Why all the trailing slashes in the URL configuration?