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.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
