Code review comment for ~ddstreet/maas:lp1896684

Revision history for this message
Dan Streetman (ddstreet) wrote :

> Thanks for finding and fixing this!
>
> This seems to fix the bug for static IP address, have you tested DHCP? Looking
> at the code[1] I think its correct but it would be good to verify.

It does look like it needed a change, i included that in the latest patch

> Couple of
> other things
>
> 1. This needs commit message to land. It should start with "LP: #1896684 - "

ok, done

> 2. Unit tests need to be added to ensure we don't regress this.

if these changes look ok to you, i'll work on adding unit tests for them

> 3. One comment inline below.

response inline (with the old diff) - basically i'm confused as to why there's a in-vlan restriction but no in-subnet restriction, without a gateway (or defined route) a node can't access a separate subnet, even if it's in the same vlan. and with a gateway (or defined route), there's no need for the separate subnet to be in the same vlan.

>
>
> [1] https://git.launchpad.net/maas/tree/src/maasserver/dhcp.py#n420

« Back to merge proposal