Fixup code review comments

7 files changed, 109 insertions, 125 deletions

diff --git a/cloudinit/distros/debian.py b/cloudinit/distros/debian.py
index e71aaa97..511ed4fe 100644
--- a/cloudinit/distros/debian.py
+++ b/cloudinit/distros/debian.py
@@ -57,7 +57,11 @@ class Distro(distros.Distro):
         # should only happen say once per instance...)
         self._runner = helpers.Runners(paths)
         self.osfamily = 'debian'
-        self._net_renderer = eni.Renderer()
+        self._net_renderer = eni.Renderer({
+            'eni_path': self.network_conf_fn,
+            'links_prefix_path': self.links_prefix,
+            'netrules_path': None,
+        })
 
     def apply_locale(self, locale, out_fn=None):
         if not out_fn:
@@ -82,12 +86,8 @@ class Distro(distros.Distro):
 
     def _write_network_config(self, netconfig):
         ns = net.parse_net_config_data(netconfig)
-        self._net_renderer.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("/", ns)
         _maybe_remove_legacy_eth0()
-
         return []
 
     def _bring_up_interfaces(self, device_names):
diff --git a/cloudinit/distros/rhel.py b/cloudinit/distros/rhel.py
index 20525e47..1aa42d75 100644
--- a/cloudinit/distros/rhel.py
+++ b/cloudinit/distros/rhel.py
@@ -67,8 +67,8 @@ class Distro(distros.Distro):
         self.package_command('install', pkgs=pkglist)
 
     def _write_network_config(self, netconfig):
-        self._net_renderer.render_network_state(
-            target="/", parse_net_config_data(netconfig))
+        ns = parse_net_config_data(netconfig)
+        self._net_renderer.render_network_state("/", ns)
         return []
 
     def _write_network(self, settings):
diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index f5668fff..6959ad34 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -26,7 +26,6 @@ from cloudinit import util
 LOG = logging.getLogger(__name__)
 SYS_CLASS_NET = "/sys/class/net/"
 DEFAULT_PRIMARY_INTERFACE = 'eth0'
-LINKS_FNAME_PREFIX = "etc/systemd/network/50-cloud-init-"
 
 
 def sys_dev_path(devname, path=""):
diff --git a/cloudinit/net/eni.py b/cloudinit/net/eni.py
index df0df9b2..352f7dd5 100644
--- a/cloudinit/net/eni.py
+++ b/cloudinit/net/eni.py
@@ -16,10 +16,9 @@ import glob
 import os
 import re
 
-from . import LINKS_FNAME_PREFIX
 from . import ParserError
 
-from .udev import generate_udev_rule
+from . import renderer
 
 from cloudinit import util
 
@@ -297,9 +296,18 @@ def _ifaces_to_net_config_data(ifaces):
             'config': [devs[d] for d in sorted(devs)]}
 
 
-class Renderer(object):
+class Renderer(renderer.Renderer):
     """Renders network information in a /etc/network/interfaces format."""
 
+    def __init__(self, config=None):
+        if not config:
+            config = {}
+        self.eni_path = config.get('eni_path', 'etc/network/interfaces')
+        self.links_path_prefix = config.get(
+            'links_path_prefix', 'etc/systemd/network/50-cloud-init-')
+        self.netrules_path = config.get(
+            'netrules_path', 'etc/udev/rules.d/70-persistent-net.rules')
+
     def _render_persistent_net(self, network_state):
         """Given state, emit udev rules to map mac to ifname."""
         content = ""
@@ -419,29 +427,23 @@ class Renderer(object):
         content = content.replace('mac_address', 'hwaddress')
         return content
 
-    def render_network_state(
-        self, target, network_state, eni="etc/network/interfaces",
-            links_prefix=LINKS_FNAME_PREFIX,
-            netrules='etc/udev/rules.d/70-persistent-net.rules',
-            writer=None):
-
-        fpeni = os.path.sep.join((target, eni,))
+    def render_network_state(self, target, network_state):
+        fpeni = os.path.join(target, self.eni_path)
         util.ensure_dir(os.path.dirname(fpeni))
         util.write_file(fpeni, self._render_interfaces(network_state))
 
-        if netrules:
-            netrules = os.path.sep.join((target, netrules,))
+        if self.netrules_path:
+            netrules = os.path.join(target, self.netrules_path)
             util.ensure_dir(os.path.dirname(netrules))
             util.write_file(netrules,
                             self._render_persistent_net(network_state))
 
-        if links_prefix:
+        if self.links_path_prefix:
             self._render_systemd_links(target, network_state,
-                                       links_prefix=links_prefix)
+                                       links_prefix=self.links_path_prefix)
 
-    def _render_systemd_links(self, target, network_state,
-                              links_prefix=LINKS_FNAME_PREFIX):
-        fp_prefix = os.path.sep.join((target, links_prefix))
+    def _render_systemd_links(self, target, network_state, links_prefix):
+        fp_prefix = os.path.join(target, links_prefix)
         for f in glob.glob(fp_prefix + "*"):
             os.unlink(f)
         for iface in network_state.iter_interfaces():
diff --git a/cloudinit/net/renderer.py b/cloudinit/net/renderer.py
new file mode 100644
index 00000000..310cbe0d
--- /dev/null
+++ b/cloudinit/net/renderer.py
@@ -0,0 +1,48 @@
+#   Copyright (C) 2013-2014 Canonical Ltd.
+#
+#   Author: Scott Moser <scott.moser@canonical.com>
+#   Author: Blake Rouse <blake.rouse@canonical.com>
+#
+#   Curtin is free software: you can redistribute it and/or modify it under
+#   the terms of the GNU Affero General Public License as published by the
+#   Free Software Foundation, either version 3 of the License, or (at your
+#   option) any later version.
+#
+#   Curtin is distributed in the hope that it will be useful, but WITHOUT ANY
+#   WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+#   FOR A PARTICULAR PURPOSE.  See the GNU Affero General Public License for
+#   more details.
+#
+#   You should have received a copy of the GNU Affero General Public License
+#   along with Curtin.  If not, see <http://www.gnu.org/licenses/>.
+
+import six
+
+from .udev import generate_udev_rule
+
+
+def filter_by_type(match_type):
+    return lambda iface: match_type == iface['type']
+
+
+def filter_by_name(match_name):
+    return lambda iface: match_name == iface['name']
+
+
+filter_by_physical = filter_by_type('physical')
+
+
+class Renderer(object):
+
+    @staticmethod
+    def _render_persistent_net(network_state):
+        """Given state, emit udev rules to map mac to ifname."""
+        # TODO(harlowja): this seems shared between eni renderer and
+        # this, so move it to a shared location.
+        content = six.StringIO()
+        for iface in network_state.iter_interfaces(filter_by_physical):
+            # for physical interfaces write out a persist net udev rule
+            if 'name' in iface and iface.get('mac_address'):
+                content.write(generate_udev_rule(iface['name'],
+                                                 iface['mac_address']))
+        return content.getvalue()
diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
index 647ca3ed..dd005796 100644
--- a/cloudinit/net/sysconfig.py
+++ b/cloudinit/net/sysconfig.py
@@ -20,9 +20,10 @@ import six
 from cloudinit.distros.parsers import resolv_conf
 from cloudinit import util
 
-from . import network_state
 from .udev import generate_udev_rule
 
+from . import renderer
+
 
 def _make_header(sep='#'):
     lines = [
@@ -37,17 +38,6 @@ def _make_header(sep='#'):
     return "\n".join(lines)
 
 
-def _filter_by_type(match_type):
-    return lambda iface: match_type == iface['type']
-
-
-def _filter_by_name(match_name):
-    return lambda iface: match_name == iface['name']
-
-
-_filter_by_physical = _filter_by_type('physical')
-
-
 def _is_default_route(route):
     if route['network'] == '::' and route['netmask'] == 0:
         return True
@@ -182,7 +172,7 @@ class NetInterface(ConfigMap):
         return c
 
 
-class Renderer(object):
+class Renderer(renderer.Renderer):
     """Renders network information in a /etc/sysconfig format."""
 
     # See: https://access.redhat.com/documentation/en-US/\
@@ -211,18 +201,13 @@ class Renderer(object):
         ('bridge_bridgeprio', 'PRIO'),
     ])
 
-    @staticmethod
-    def _render_persistent_net(network_state):
-        """Given state, emit udev rules to map mac to ifname."""
-        # TODO(harlowja): this seems shared between eni renderer and
-        # this, so move it to a shared location.
-        content = six.StringIO()
-        for iface in network_state.iter_interfaces(_filter_by_physical):
-            # for physical interfaces write out a persist net udev rule
-            if 'name' in iface and iface.get('mac_address'):
-                content.write(generate_udev_rule(iface['name'],
-                                                 iface['mac_address']))
-        return content.getvalue()
+    def __init__(self, config=None):
+        if not config:
+            config = {}
+        self.sysconf_dir = config.get('sysconf_dir', 'etc/sysconfig/')
+        self.netrules_path = config.get(
+            'netrules_path', 'etc/udev/rules.d/70-persistent-net.rules')
+        self.dns_path = config.get('dns_path', 'etc/resolv.conf')
 
     @classmethod
     def _render_iface_shared(cls, iface, iface_cfg):
@@ -302,7 +287,7 @@ class Renderer(object):
 
     @classmethod
     def _render_physical_interfaces(cls, network_state, iface_contents):
-        for iface in network_state.iter_interfaces(_filter_by_physical):
+        for iface in network_state.iter_interfaces(renderer.filter_by_physical):
             iface_name = iface['name']
             iface_subnets = iface.get("subnets", [])
             iface_cfg = iface_contents[iface_name]
@@ -319,7 +304,7 @@ class Renderer(object):
 
     @classmethod
     def _render_bond_interfaces(cls, network_state, iface_contents):
-        for iface in network_state.iter_interfaces(_filter_by_type('bond')):
+        for iface in network_state.iter_interfaces(renderer.filter_by_type('bond')):
             iface_name = iface['name']
             iface_cfg = iface_contents[iface_name]
             cls._render_bonding_opts(iface_cfg, iface)
@@ -337,7 +322,7 @@ class Renderer(object):
 
     @staticmethod
     def _render_vlan_interfaces(network_state, iface_contents):
-        for iface in network_state.iter_interfaces(_filter_by_type('vlan')):
+        for iface in network_state.iter_interfaces(renderer.filter_by_type('vlan')):
             iface_name = iface['name']
             iface_cfg = iface_contents[iface_name]
             iface_cfg['VLAN'] = True
@@ -348,15 +333,15 @@ class Renderer(object):
         content = resolv_conf.ResolvConf("")
         if existing_dns_path and os.path.isfile(existing_dns_path):
             content = resolv_conf.ResolvConf(util.load_file(existing_dns_path))
-        for ns in network_state.dns_nameservers:
-            content.add_nameserver(ns)
-        for d in network_state.dns_searchdomains:
-            content.add_search_domain(d)
+        for nameserver in network_state.dns_nameservers:
+            content.add_nameserver(nameserver)
+        for searchdomain in network_state.dns_searchdomains:
+            content.add_search_domain(searchdomain)
         return "\n".join([_make_header(';'), str(content)])
 
     @classmethod
     def _render_bridge_interfaces(cls, network_state, iface_contents):
-        for iface in network_state.iter_interfaces(_filter_by_type('bridge')):
+        for iface in network_state.iter_interfaces(renderer.filter_by_type('bridge')):
             iface_name = iface['name']
             iface_cfg = iface_contents[iface_name]
             iface_cfg.kind = 'bridge'
@@ -397,67 +382,17 @@ class Renderer(object):
                 contents[iface_cfg.routes.path] = iface_cfg.routes.to_string()
         return contents
 
-    def render_network_state(
-            self, target, network_state, sysconf_dir="etc/sysconfig/",
-            netrules='etc/udev/rules.d/70-persistent-net.rules',
-            dns='etc/resolv.conf'):
-        if target:
-            base_sysconf_dir = os.path.join(target, sysconf_dir)
-        else:
-            base_sysconf_dir = sysconf_dir
+    def render_network_state(self, target, network_state):
+        base_sysconf_dir = os.path.join(target, self.sysconf_dir)
         for path, data in self._render_sysconfig(base_sysconf_dir,
                                                  network_state).items():
-            if target:
-                util.write_file(path, data)
-            else:
-                print("File to be at: %s" % path)
-                print(data)
-        if dns:
-            if target:
-                dns_path = os.path.join(target, dns)
-                resolv_content = self._render_dns(network_state,
-                                                  existing_dns_path=dns_path)
-                util.write_file(dns_path, resolv_content)
-            else:
-                resolv_content = self._render_dns(network_state)
-                dns_path = dns
-                print("File to be at: %s" % dns_path)
-                print(resolv_content)
-        if netrules:
+            util.write_file(path, data)
+        if self.dns_path:
+            dns_path = os.path.join(target, self.dns_path)
+            resolv_content = self._render_dns(network_state,
+                                              existing_dns_path=dns_path)
+            util.write_file(dns_path, resolv_content)
+        if self.netrules_path:
             netrules_content = self._render_persistent_net(network_state)
-            if target:
-                netrules_path = os.path.join(target, netrules)
-                util.write_file(netrules_path, netrules_content)
-            else:
-                netrules_path = netrules
-                print("File to be at: %s" % netrules_path)
-                print(netrules_content)
-
-
-def main():
-    """Reads a os network state json file and outputs what would be written."""
-    from cloudinit.sources.helpers import openstack
-
-    import argparse
-    import json
-
-    parser = argparse.ArgumentParser()
-    parser.add_argument("-f", "--file", metavar="FILE",
-                        help=("openstack network json file"
-                              " to read (required)"),
-                        required=True)
-    parser.add_argument("-d", "--dir", metavar="DIR",
-                        help=("directory to write output into (if"
-                              " not provided then written to stdout)"),
-                        default=None)
-    args = parser.parse_args()
-
-    network_json = json.loads(util.load_file(args.file))
-    net_state = network_state.parse_net_config_data(
-        openstack.convert_net_json(network_json), skip_broken=False)
-    r = Renderer()
-    r.render_network_state(args.dir, net_state)
-
-
-if __name__ == '__main__':
-    main()
+            netrules_path = os.path.join(target, self.netrules_path)
+            util.write_file(netrules_path, netrules_content)
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index 43602030..3ae00fc6 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -248,11 +248,11 @@ class TestEniNetRendering(TestCase):
         render_dir = os.path.join(tmp_dir, "render")
         os.makedirs(render_dir)
 
-        renderer = eni.Renderer()
-        renderer.render_network_state(render_dir, ns,
-                                      eni="interfaces",
-                                      links_prefix=None,
-                                      netrules=None)
+        renderer = eni.Renderer(
+            {'links_path_prefix': None,
+             'eni_path': 'interfaces', 'netrules_path': None,
+             })
+        renderer.render_network_state(render_dir, ns)
 
         self.assertTrue(os.path.exists(os.path.join(render_dir,
                                                     'interfaces')))