Code review comment for lp:~julian-edwards/maas/static-range-in-ui-api

Thanks for the review!

On 10/06/14 17:50, Raphaël Badin wrote:
> Review: Approve
>
> Looks good. See my comments inline; I guess we will add validation for the new fields in a follow-up branch (check that the static range doesn't overlap with other ip ranges [static or dynamic])…?

Correct, jtv said he'd look at that.

>> class NodeGroupInterfacesHandler(OperationsHandler):
>> @@ -1837,10 +1839,16 @@
>> :type broadcast_ip: unicode (IP Address)
>> :param router_ip: Address of default gateway.
>> :type router_ip: unicode (IP Address)
>> - :param ip_range_low: Lowest IP address to assign to clients.
>> + :param ip_range_low: Lowest dynamic IP address to assign to clients.
>> :type ip_range_low: unicode (IP Address)
>> - :param ip_range_high: Highest IP address to assign to clients.
>> + :param ip_range_high: Highest dynamic IP address to assign to clients.
>> :type ip_range_high: unicode (IP Address)
>> + :param static_ip_range_low: Lowest static IP address to assign to
>> + clients.
>> + :type static_ip_range_low: unicode (IP Address)
>> + :param static_ip_range_high: Highest static IP address to assign to
>> + clients.
>> + :type static_ip_range_high: unicode (IP Address)
>
> This is probably worth testing (see src/maasserver/tests/test_api.py:TestNodeGroupInterfaceAPI and TestNodeGroupInterfacesAPI.test_new_creates_interface).

It's already tested with the existing tests, they iterate over the list
of fields supplied!

>> + if static_ip_range_low is None or static_ip_range_high is None:
>> + static_low, static_high = self.make_ip_range()
>
> AFAIK, we've always worked under the assumption that the static range (same as the dynamic range) was going to be *inside* the nodegroupinterface's network. Here you're creating a completely different network range possibility outside of the nodegroupinterface's network: although this is testing code I think we should change this to better reflect what happens in reality.

Yes. However I am thinking ahead here. If we assume it's not in the
same range and make the code work for that now, it'll also just DTRT
when we redefine the ranges later.