Code review comment for lp:~allenap/maas/bug-1081701-b

Looks generally ok but I'm afraid I've got a few questions about the specifics of this branch :).

[0]

8 - form.save()
9 + nodegroup = form.save()
10 + update_nodegroup_maas_url(nodegroup, request)

I don't really see the need for that: the maas_url field will be updated when the cluster will have been accepted and will try to connect again so why bother updating it also when the nodegroup is created (also, at this stage, this might be a malicious request)?

[1]

23 elif existing_nodegroup.status == NODEGROUP_STATUS.PENDING:
24 + update_nodegroup_maas_url(existing_nodegroup, request)

Again, why the need to update nodegroup.maas_url here? We only need it after the cluster controller has been accepted (and tries to connect) and that is already taken care of by the code you've added after " if existing_nodegroup.status == NODEGROUP_STATUS.ACCEPTED:".

[3]

We need to have a test to make sure that when the master nodegroup registers itself, the value of maas_url is not changed and is still the empty string. This is fairly important since the master nodegroup runs on the same machine as the region controller and is configured to reach the region controller through the localhost interface. When dealing with the master nodegroup, we need to use DEFAULT_MAAS_URL where we would have used nodegroup.maas_url otherwise. All the code I've written in the fixes for c), d) and e) assumes that if nodegroup.maas_url is '', then DEFAULT_MAAS_URL should be used.

[4]

49 + self.assertIn(
50 + response.status_code,
51 + {code for code in httplib.responses if code // 100 == 2})

If this fails, the error message is not going to be really explicit, it would be good to have the content of the response in the failure message.

[5]

116 +class TestUpdateNodeGroupMAASURL(TransactionTestCase):
The test for update_nodegroup_maas_url should be in a transaction.

Why? I don't see anything in there that justifies the usage of a TransactionTestCase over the regular (Django-based) TestCase.