cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

Josh,

Thanks for your work and cleanup on this.
My concerns at the moment are
a.) large churn on code and i need to get something into 16.04 to fix some bugs (bug 1577982, bug 1579130, bug 1577844), so i'd like to hold off on this for that.

b.) if we want 'net' to be standalone then i'd prefer for it to not have 'from cloudinit import...' as that indicates reliance on cloudinit or at least some required conversion before external use. you have experience with this through oslo so i guess i'm fine if there is a sane path forward.

c.) non-standard library usage in 'net'.  even if this is just six I will need to support curtin running on Ubuntu 12.04 (python-six at 1.1) for the next 12 months at least.

some nitpicks inline below.


Diff comments:

> 
> === modified file 'cloudinit/distros/debian.py'
> --- cloudinit/distros/debian.py	2016-05-12 17:56:26 +0000
> +++ cloudinit/distros/debian.py	2016-05-24 19:27:41 +0000
> @@ -80,10 +82,10 @@
>  
>      def _write_network_config(self, netconfig):
>          ns = net.parse_net_config_data(netconfig)

below is white space only churn ?

> -        net.render_network_state(target="/", network_state=ns,
> -                                 eni=self.network_conf_fn,
> -                                 links_prefix=self.links_prefix,
> -                                 netrules=None)
> +        self._net_renderer.render_network_state(
> +            target="/", network_state=ns,
> +            eni=self.network_conf_fn, links_prefix=self.links_prefix,
> +            netrules=None)
>          _maybe_remove_legacy_eth0()
>  
>          return []
> 
> === modified file 'cloudinit/net/network_state.py'
> --- cloudinit/net/network_state.py	2016-05-12 17:56:26 +0000
> +++ cloudinit/net/network_state.py	2016-05-24 19:27:41 +0000
> @@ -295,15 +338,8 @@
>  
>          interfaces.update({iface['name']: iface})
>  
> +    @ensure_command_keys(['address'])

this is pretty nice!

>      def handle_nameserver(self, command):
> -        required_keys = [
> -            'address',
> -        ]
> -        if not self.valid_command(command, required_keys):
> -            print('Skipping Invalid command: {}'.format(command))
> -            print(self.dump_network_state())
> -            return
> -
>          dns = self.network_state.get('dns')
>          if 'address' in command:
>              addrs = command['address']
> 
> === modified file 'cloudinit/util.py'
> --- cloudinit/util.py	2016-05-12 17:56:26 +0000
> +++ cloudinit/util.py	2016-05-24 19:27:41 +0000
> @@ -1696,7 +1705,8 @@
>          sp = subprocess.Popen(args, **kws)
>          (out, err) = sp.communicate(data)
>      except OSError as e:
> -        raise ProcessExecutionError(cmd=args, reason=e)
> +        raise ProcessExecutionError(cmd=args, reason=e,

pointless line break?

> +                                    errno=e.errno)
>      rc = sp.returncode
>      if rc not in rcs:
>          raise ProcessExecutionError(stdout=out, stderr=err,
> 
> === modified file 'requirements.txt'
> --- requirements.txt	2015-01-26 21:37:29 +0000
> +++ requirements.txt	2016-05-24 19:27:41 +0000
> @@ -11,8 +11,12 @@
>  oauthlib
>  
>  # This one is currently used only by the CloudSigma and SmartOS datasources.
> -# If these datasources are removed, this is no longer needed
> -pyserial
> +# If these datasources are removed, this is no longer needed.
> +#
> +# This will not work in py2.6 so it is only optionally installed on
> +# python 2.7 and later.
> +#
> +# pyserial

doesnt this look wrong? i'd rather not just hide the dependency.

>  
>  # This is only needed for places where we need to support configs in a manner
>  # that the built-in config parser is not sufficent (ie


-- 
https://code.launchpad.net/~harlowja/cloud-init/cloud-init-net-refactor/+merge/293957
Your team cloud init development team is requested to review the proposed merge of lp:~harlowja/cloud-init/cloud-init-net-refactor into lp:cloud-init.