Code review comment for lp:~raharper/curtin/trunk.vmtest-remove-bug-timers

On Thu, Oct 5, 2017 at 1:38 PM, Scott Moser <email address hidden> wrote:

>
>
> Diff comments:
>
> >
> > === modified file 'examples/tests/network_alias.yaml'
> > --- examples/tests/network_alias.yaml 2017-06-13 22:10:10 +0000
> > +++ examples/tests/network_alias.yaml 2017-10-05 16:35:19 +0000
> > @@ -9,11 +9,9 @@
> > subnets:
> > - type: static
> > address: 10.47.98.1/24
> > - mtu: 1501
> > - type: static
> > address: 2001:4800:78ff:1b:be76:4eff:fe06:ffac
> > netmask: 'ffff:ffff:ffff:ffff::'
> > - mtu: 1480
>
> you say "not needed" in your commit message, but why?
> and isnt it concerning that this diddnt have fallout in tests ?
>

It does have fallout for netplan which doesn;'t support ipv6 mtu;
however, the *scope* of this test is multiple ip address per interface
The mtu itself has no affect on assigning multiple addresses to a single
interface.

The failure to apply an ipv6 MTU in this test is not germane; we're
only testing if we can set multiple ips.

>
> > # multi_v4_alias: multiple v4 addrs on same interface
> > - type: physical
> > name: interface1
> >
> > === modified file 'tests/vmtests/test_network.py'
> > --- tests/vmtests/test_network.py 2017-08-02 15:46:35 +0000
> > +++ tests/vmtests/test_network.py 2017-10-05 16:35:19 +0000
> > @@ -285,14 +285,19 @@
> > ip_route_show = self.load_collect_file("ip_route_show")
> > logger.debug("ip route show:\n{}".format(ip_route_show))
> > for line in [line for line in ip_route_show.split('\n')
> > - if 'src' in line]:
> > + if 'src' in line and not
> line.startswith('default')]:
> > + print('ip_route_show: line: %s' % line)
> > m = re.search(r'^(?P<network>\S+)\sdev\s' +
> > r'(?P<devname>\S+)\s+' +
> > - r'proto kernel\s+scope link' +
> > - r'\s+src\s(?P<src_ip>\S+)',
> > + r'proto\s(?P<proto>\S+)\s+' +
> > + r'scope\s(?P<scope>\S+)\s+' +
> > + r'src\s(?P<src_ip>\S+)',
> > line)
> > - route_info = m.groupdict('')
> > - logger.debug(route_info)
> > + if m:
> > + route_info = m.groupdict('')
> > + logger.debug(route_info)
> > + else:
> > + raise ValueError('Failed match ip_route_show line: %s'
> % line)
>
> bikeshed: this is probably more of a RuntimeException
> kind of "got stuff i didnt expect".
> doesnt matter. take input or not.
>

Sure

>
> >
> > routes = {
> > '4': route_n,
> > @@ -390,7 +395,8 @@
> > gateways.append(subnet.get('gateway'))
> > for route in subnet.get('routes', []):
> > gateways += __find_gw_config(route)
> > - return gateways
> > + # drop duplicate gateways (static routes)
>
> this does have the side affect of sorting your list.
>

That's ok, we iterate through the whole list, each item is
checked if it's found in the entire output.

> > + return list(set(gateways))
> >
> > # handle gateways by looking at routing table
> > configured_gws = __find_gw_config(subnet)
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.vmtest-
> remove-bug-timers/+merge/331757
> You are the owner of lp:~raharper/curtin/trunk.vmtest-remove-bug-timers.
>