Add the vlan_mask fixture #14
Conversation
| @@ -84,9 +84,11 @@ def addresses(self): | |||
| if addr.is_link_local: | |||
| continue | |||
| elif addr.version == 4: | |||
| data.setdefault(socket.AF_INET, []).append(addr) | |||
| values = data.setdefault(socket.AF_INET, []) | |||
| values.append(addr), values.append(prefix) | |||
There was a problem hiding this comment.
You can't do this. This is a list for a reason - its possible to have multiple addresses per network interface. You can't just shove whatever extra data you want in here just because.
There was a problem hiding this comment.
addr is already an ipaddress.IPv4Address/ipaddress.IPv6Address - we use ipaddress.ip_address to parse it.
It would make much more sense to just mixin the prefix information and use ipaddress.ip_interface instead.
There was a problem hiding this comment.
Actually, we probably should just mark this method as deprecated and introduce a new method that returns interfaces instead of addresses.
| @@ -84,9 +84,11 @@ def addresses(self): | |||
| if addr.is_link_local: | |||
| continue | |||
| elif addr.version == 4: | |||
| data.setdefault(socket.AF_INET, []).append(addr) | |||
| values = data.setdefault(socket.AF_INET, []) | |||
| values.append(addr), values.append(prefix) | |||
There was a problem hiding this comment.
addr is already an ipaddress.IPv4Address/ipaddress.IPv6Address - we use ipaddress.ip_address to parse it.
It would make much more sense to just mixin the prefix information and use ipaddress.ip_interface instead.
| @pytest.fixture(scope='class') | ||
| def vlan_mask(vlan, addr_family): | ||
| '''Return the correct network mask for the macvlan interface''' | ||
| return vlan.get_address(addr_family)[1] |
There was a problem hiding this comment.
First, this isn't the mask - its the prefix.
And I don't think you should add this fixture.
vlan and vlan_set have been the predominant way to get macvlan(s). Your change to get_address breaks all code that uses either of these two fixtures directly. Realise that vlan_addr is just a convenience fixture because of the sheer number of tests that only care about having a second IP address.
But we have a lot of tests that ask for 5 more, for example, and this change will break them.
| @@ -84,9 +84,11 @@ def addresses(self): | |||
| if addr.is_link_local: | |||
| continue | |||
| elif addr.version == 4: | |||
| data.setdefault(socket.AF_INET, []).append(addr) | |||
| values = data.setdefault(socket.AF_INET, []) | |||
| values.append(addr), values.append(prefix) | |||
There was a problem hiding this comment.
Actually, we probably should just mark this method as deprecated and introduce a new method that returns interfaces instead of addresses.
The vlan mask info is missing before and it is needed in some tests.