Merge lp://qastaging/~rvb/maas/maas-node-api into lp://qastaging/~maas-committers/maas/trunk

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.assertEqua...

Thanks for the thorough review Gavin!

> [1]

Okay.

> [2]
> + for k, v in error.message_dict.items():
> s/items/iteritems/

Nice.

> [3]
> + if type(v) is list:
> if isinstance(v, list):

k

> [4]
> + return get_object_or_404(Node, system_id=system_id)
> Django is a massive layer violation isn't it? ;)

Well, you'll admit it's convenient and avoids boiler plate code…

> [5]
> + 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)?

I think it's clearer like it is.

> [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...

Good spot. I wonder why I use pyflake when I have pynella :)

> [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.

Well, this is only required because this is not checked at the model level, but I suppose we could put that in the model.

If you don't mind, I'll leave it like this for now because I think we will have to revisit that shortly anyway.

> [8]
> How do we layer authorization onto this?

That's my next task :)

> [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.

Good point, Django agrees with you: https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.Field.null

> [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.

I agree. Model validation was only added in Django 1.2. Before that the validation was done using Forms. I think the reason for the manual call to full_clean is historic… but I might be wrong.

> [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.

True, I've been infected by the lp way to name methods.

> [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.

Okay, let's do this properly since we are...