Changed to use template files #13

mikemrm · 2019-10-31T17:24:13Z

Here I'm pulling the hard coded """{template}""" into actual
template files. In doing so, I also brought common files that
are shared between network configs such has hostname configuration
and hosts files into the distro builder.

By doing so, I also had to move the render and execution functions
into the distro builder. This I think makes more sense, and allows
for a better hierarchical structure to tasks.

To better assist with adding tasks and using template files, a new
parent class Tasks was created which allows a task to be set by
calling self.task(task, content, write_mode, mode, fmt) which
will build the structure accordingly. For a template file you can
call self.task_template(task, rel_path, write_mode, mode, fmt).
rel_path is relative to the templates_base variable defined
in the distro builder file.

mmlb · 2019-10-31T18:16:57Z

packetnetworking/distros/debian/conftest.py

+            if not isinstance(builder.builders[j], DebianBondedNetwork):
+                del builder.builders[j]
+            j -= 1
+        return builder


I didn't like the way this looked and went looking for some better looking code, but I only came up with:

builder.builders = next(builder for builders in builder.builders if isinstance(builder, DebianBondedNetwork)) return builder

and I'm not so sure its better. It does have the benefit of not needing to iterate through the whole list only to keep 1 item though.

Hah, good catch, much cleaner

minus the typo. It should be

builder.builders = next(builder for builder in builder.builders if isinstance(builder, DebianBondedNetwork)) return builder

mmlb · 2019-10-31T18:20:16Z

packetnetworking/distros/debian/templates/bonded/etc_network_interfaces.j2

+{% endif %}
+    bond-master bond0
+{% endfor %}
+{% endif %}


mmlb · 2019-10-31T18:20:28Z

packetnetworking/distros/debian/templates/individual/etc_network_interfaces.j2

+    post-up route add -net {{ subnet }} gw {{ ip4priv.gateway }}
+    post-down route del -net {{ subnet }} gw {{ ip4priv.gateway }}
+    {% endfor %}
+{% endif %}


mmlb · 2019-10-31T18:21:08Z

packetnetworking/distros/distro_builder.py

+import logging
+from textwrap import dedent
+from jinja2 import Template, StrictUndefined
+from ..utils import Tasks


split imports with a blank line between stdlib, 3party, this-repo

packetnetworking/distros/distro_builder.py

mmlb · 2019-10-31T18:28:53Z

packetnetworking/distros/redhat/bonded.py

+            self.task_template(
+                "etc/sysconfig/network-scripts/ifcfg-bond0:0",
+                "bonded/etc_sysconfig_network-scripts_ifcfg-bond0_0.j2",
+            )

            # If no ip4pub is specified, the ip4priv is configured on the bond0 interface


this comment isn't really very useful here, maybe instead at the top of inside the if and expanded to say bond0 will already by the private ip so we don't need bond0:0 and we don't need any extra route either?

mmlb · 2019-10-31T18:31:53Z

packetnetworking/distros/redhat/builder.py

@@ -11,3 +11,13 @@ class RedhatBuilder(DistroBuilder):
        "scientificcernslc",
    ]
    network_builders = [RedhatBondedNetwork, RedhatIndividualNetwork]
+    templates_base = "distros/redhat/templates"


it would kind of be better of templates was assumed to exist or that this os's templates are in distro/$distro/templates and handle existence/non-existence of templates in the base class machinery

I thought of that, but went with this to be lazy. Will go ahead and implement that.

mmlb · 2019-10-31T18:34:30Z

packetnetworking/distros/redhat/builder.py

+    def build_tasks(self):
+        self.tasks = {}
+
+        self.task_template("etc/hostname", "etc_hostname.j2")


evil me want to find some utf-8 char that looks like / and use that in the template name and replace utf8-esq-/ -> / and have the base class be able to handle these common cases :D Less duplicated code.

https://www.fileformat.info/info/unicode/char/2215/index.htm :D

I personally like having all the files in the same folder like this. But I could build out the actual structure. There's nothing keeping us from doing that.

so with the unicode char hack it would still all be in one dir, its just that we would have a character that we could use to encode filepaths in the filename. Hence the evil part :p

mmlb · 2019-10-31T18:40:21Z

packetnetworking/distros/test_distro_builder.py

@@ -1,4 +1,7 @@
 from .distro_builder import DistroBuilder, get_distro_builder
+from ..builder import NetworkData
+from .. import utils


these imports are in the wrong order right?

heh, not sure, flipped

mmlb · 2019-10-31T18:40:59Z

setup.py

@@ -1,7 +1,25 @@
 #!/usr/bin/env python3

+import os
 from setuptools import setup, find_packages


isn't this a nono?

import os? github has been selecting wrong lines. Or are you referring to package_dir = ?

nope to the importing multiple things in one line

Have never heard multiple imports under a package on the same line being a problem. Quick google search hasn't brought anything up about it.

right you are. PEP8 explicitly allows this. Its the multiple different modules in 1 line that is a nono. Also google python style guide says

Do not use relative names in imports. Even if the module is in the same package, use the full package name. This helps prevent unintentionally importing a package twice.

But who cares.

mmlb · 2019-10-31T19:51:23Z

packetnetworking/distros/distro_builder.py


    def build(self):
+        """
+        Build executes the `DistroBuilder.build_tasks` and each `NetworkBuilder.build`
+        function which builds the `tasks` dictionary of files to be processed


but what does this mean? LIke how is build different than render?

I've given this another shot.

mmlb · 2019-10-31T19:53:02Z

packetnetworking/distros/redhat/builder.py

+    def build_tasks(self):
+        self.tasks = {}
+
+        self.task_template("etc/hostname", "etc_hostname.j2")


so with the unicode char hack it would still all be in one dir, its just that we would have a character that we could use to encode filepaths in the filename. Hence the evil part :p

mmlb · 2019-10-31T19:54:30Z

packetnetworking/utils.py

@@ -1,9 +1,12 @@
+import os
 import re
 import sys
 import json


click should be on its own line, and please2sort

mmlb · 2019-10-31T19:55:39Z

setup.py

@@ -1,7 +1,25 @@
 #!/usr/bin/env python3

+import os
 from setuptools import setup, find_packages


nope to the importing multiple things in one line

Here I'm pulling the hard coded """{template}""" into actual template files. In doing so, I also brought common files that are shared between network configs such has hostname configuration and hosts files into the distro builder. By doing so, I also had to move the render and execution functions into the distro builder. This I think makes more sense, and allows for a better hierarchical structure to tasks. To better assist with adding tasks and using template files, a new parent class `Tasks` was created which allows a task to be set by calling `self.task(task, content, write_mode, mode, fmt)` which will build the structure accordingly. For a template file you can call `self.task_template(task, rel_path, write_mode, mode, fmt)`. `rel_path` is relative to the `templates_base` variable defined in the distro builder file.

pin black version

b4bd765

mmlb approved these changes Oct 31, 2019

View reviewed changes

mikemrm force-pushed the change-to-user-template-files branch 2 times, most recently from 9643767 to bd4275d Compare October 31, 2019 19:40

mikemrm requested a review from mmlb October 31, 2019 19:41

mmlb approved these changes Oct 31, 2019

View reviewed changes

mikemrm force-pushed the change-to-user-template-files branch from bd4275d to c8665b7 Compare October 31, 2019 20:09

mikemrm requested a review from mmlb October 31, 2019 20:10

mikemrm merged commit 07f1a19 into master Nov 6, 2019

mikemrm deleted the change-to-user-template-files branch November 6, 2019 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed to use template files #13

Changed to use template files #13

mikemrm commented Oct 31, 2019

mmlb Oct 31, 2019

mikemrm Oct 31, 2019

mmlb Oct 31, 2019

mikemrm Oct 31, 2019

mmlb Oct 31, 2019

mmlb Oct 31, 2019

mmlb Oct 31, 2019

mmlb Oct 31, 2019

mmlb Oct 31, 2019

mikemrm Oct 31, 2019

mikemrm Oct 31, 2019

mmlb Oct 31, 2019

mikemrm Oct 31, 2019

mmlb Oct 31, 2019

mmlb Oct 31, 2019

mikemrm Oct 31, 2019

mmlb Oct 31, 2019

mikemrm Oct 31, 2019

mmlb Oct 31, 2019

mikemrm Oct 31, 2019

mmlb Oct 31, 2019

mmlb Oct 31, 2019

mikemrm Oct 31, 2019

mmlb Oct 31, 2019

mmlb Oct 31, 2019

mmlb Oct 31, 2019

Changed to use template files #13

Changed to use template files #13

Conversation

mikemrm commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment