From 92c3b135a64f3ebe0f4ee3433b4583449bf8f3a8 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Jan 2022 11:38:11 -0700 Subject: [PATCH 1/5] test optimization --- cloudinit/templater.py | 64 +++++++++++++++++++++++++ tests/unittests/test_render_cloudcfg.py | 60 +++++++++++++++++++++-- tools/render-cloudcfg | 42 +++------------- 3 files changed, 126 insertions(+), 40 deletions(-) diff --git a/cloudinit/templater.py b/cloudinit/templater.py index c215bbbbd22..224fdd9e4ef 100644 --- a/cloudinit/templater.py +++ b/cloudinit/templater.py @@ -10,8 +10,10 @@ # # This file is part of cloud-init. See LICENSE file for license information. +import sys import collections import re +import argparse try: from Cheetah.Template import Template as CTemplate @@ -32,6 +34,7 @@ from cloudinit import log as logging from cloudinit import type_utils as tu from cloudinit import util +from cloudinit.atomic_helper import write_file LOG = logging.getLogger(__name__) TYPE_MATCHER = re.compile(r"##\s*template:(.*)", re.I) @@ -180,4 +183,65 @@ def render_string(content, params): return renderer(content, params) +def render_cloudcfg(argv): + VARIANTS = [ + "almalinux", + "alpine", + "amazon", + "arch", + "centos", + "cloudlinux", + "debian", + "eurolinux", + "fedora", + "freebsd", + "miraclelinux", + "netbsd", + "openbsd", + "openEuler", + "photon", + "rhel", + "suse", + "rocky", + "ubuntu", + "unknown", + "virtuozzo", + ] + parser = argparse.ArgumentParser() + platform = util.system_info() + parser.add_argument( + "--variant", + default=platform["variant"], + action="store", + help="define the variant.", + choices=VARIANTS, + ) + parser.add_argument( + "template", + nargs="?", + action="store", + default="./config/cloud.cfg.tmpl", + help="Path to the cloud.cfg template", + ) + parser.add_argument( + "output", + nargs="?", + action="store", + default="-", + help="Output file. Use '-' to write to stdout", + ) + + args = parser.parse_args(argv) + + with open(args.template, "r") as fh: + contents = fh.read() + tpl_params = {"variant": args.variant} + contents = (render_string(contents, tpl_params)).rstrip() + "\n" + util.load_yaml(contents) + if args.output == "-": + sys.stdout.write(contents) + else: + write_file(args.output, contents, omode="w") + + # vi: ts=4 expandtab diff --git a/tests/unittests/test_render_cloudcfg.py b/tests/unittests/test_render_cloudcfg.py index 81110e61977..46f4be16a6f 100644 --- a/tests/unittests/test_render_cloudcfg.py +++ b/tests/unittests/test_render_cloudcfg.py @@ -4,7 +4,7 @@ import pytest -from cloudinit import subp, util +from cloudinit import subp, util, templater from tests.unittests.helpers import cloud_init_project_dir # TODO(Look to align with tools.render-cloudcfg or cloudinit.distos.OSFAMILIES) @@ -32,10 +32,52 @@ class TestRenderCloudCfg: cmd = [sys.executable, cloud_init_project_dir("tools/render-cloudcfg")] tmpl_path = cloud_init_project_dir("config/cloud.cfg.tmpl") + def test_variant_sets_distro_in_cloud_cfg_subp(self, tmpdir): + outfile = tmpdir.join("outcfg").strpath + + subp.subp(self.cmd + ["--variant", "ubuntu", self.tmpl_path, outfile]) + with open(outfile) as stream: + system_cfg = util.load_yaml(stream.read()) + assert system_cfg["system_info"]["distro"] == "ubuntu" + + def test_variant_sets_default_user_in_cloud_cfg_subp(self, tmpdir): + outfile = tmpdir.join("outcfg").strpath + templater.render_cloudcfg( + ["--variant", "ubuntu", self.tmpl_path, outfile] + ) + with open(outfile) as stream: + system_cfg = util.load_yaml(stream.read()) + + default_user_exceptions = { + "amazon": "ec2-user", + "debian": "ubuntu", + "unknown": "ubuntu", + } + default_user = system_cfg["system_info"]["default_user"]["name"] + assert default_user == default_user_exceptions.get("ubuntu", "ubuntu") + + def test_variant_sets_network_renderer_priority_in_cloud_cfg_subp( + self, tmpdir + ): + outfile = tmpdir.join("outcfg").strpath + templater.render_cloudcfg( + ["--variant", "openbsd", self.tmpl_path, outfile] + ) + with open(outfile) as stream: + system_cfg = util.load_yaml(stream.read()) + + assert ["openbsd"] == system_cfg["system_info"]["network"]["renderers"] + @pytest.mark.parametrize("variant", (DISTRO_VARIANTS)) def test_variant_sets_distro_in_cloud_cfg(self, variant, tmpdir): + """Testing parametrized inputs with imported function saves ~0.5s per + call versus calling as subp + """ outfile = tmpdir.join("outcfg").strpath - subp.subp(self.cmd + ["--variant", variant, self.tmpl_path, outfile]) + + templater.render_cloudcfg( + ["--variant", variant, self.tmpl_path, outfile] + ) with open(outfile) as stream: system_cfg = util.load_yaml(stream.read()) if variant == "unknown": @@ -44,8 +86,13 @@ def test_variant_sets_distro_in_cloud_cfg(self, variant, tmpdir): @pytest.mark.parametrize("variant", (DISTRO_VARIANTS)) def test_variant_sets_default_user_in_cloud_cfg(self, variant, tmpdir): + """Testing parametrized inputs with imported function saves ~0.5s per + call versus calling as subp + """ outfile = tmpdir.join("outcfg").strpath - subp.subp(self.cmd + ["--variant", variant, self.tmpl_path, outfile]) + templater.render_cloudcfg( + ["--variant", variant, self.tmpl_path, outfile] + ) with open(outfile) as stream: system_cfg = util.load_yaml(stream.read()) @@ -69,8 +116,13 @@ def test_variant_sets_default_user_in_cloud_cfg(self, variant, tmpdir): def test_variant_sets_network_renderer_priority_in_cloud_cfg( self, variant, renderers, tmpdir ): + """Testing parametrized inputs with imported function saves ~0.5s per + call versus calling as subp + """ outfile = tmpdir.join("outcfg").strpath - subp.subp(self.cmd + ["--variant", variant, self.tmpl_path, outfile]) + templater.render_cloudcfg( + ["--variant", variant, self.tmpl_path, outfile] + ) with open(outfile) as stream: system_cfg = util.load_yaml(stream.read()) diff --git a/tools/render-cloudcfg b/tools/render-cloudcfg index 6642bd5897c..93dafd3c5b2 100755 --- a/tools/render-cloudcfg +++ b/tools/render-cloudcfg @@ -1,47 +1,17 @@ #!/usr/bin/env python3 -import argparse import os import sys -VARIANTS = ["almalinux", "alpine", "amazon", "arch", "centos", "cloudlinux", "debian", - "eurolinux", "fedora", "freebsd", "miraclelinux", "netbsd", "openbsd", "openEuler", "photon", - "rhel", "suse","rocky", "ubuntu", "unknown", "virtuozzo"] - - -if "avoid-pep8-E402-import-not-top-of-file": - _tdir = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) - sys.path.insert(0, _tdir) - from cloudinit import templater - from cloudinit import util - from cloudinit.atomic_helper import write_file - def main(): - parser = argparse.ArgumentParser() - platform = util.system_info() - parser.add_argument( - "--variant", default=platform['variant'], action="store", - help="define the variant.", choices=VARIANTS) - parser.add_argument( - "template", nargs="?", action="store", - default='./config/cloud.cfg.tmpl', - help="Path to the cloud.cfg template") - parser.add_argument( - "output", nargs="?", action="store", default="-", - help="Output file. Use '-' to write to stdout") + if "avoid-pep8-E402-import-not-top-of-file": + _tdir = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) + sys.path.insert(0, _tdir) + from cloudinit import templater - args = parser.parse_args() + templater.render_cloudcfg(sys.argv[1:]) - with open(args.template, 'r') as fh: - contents = fh.read() - tpl_params = {'variant': args.variant} - contents = (templater.render_string(contents, tpl_params)).rstrip() + "\n" - util.load_yaml(contents) - if args.output == "-": - sys.stdout.write(contents) - else: - write_file(args.output, contents, omode="w") -if __name__ == '__main__': +if __name__ == "__main__": main() From 822b936142fede3fd3c35085387281db55a9bfd5 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Jan 2022 11:46:16 -0700 Subject: [PATCH 2/5] optimization --- tests/unittests/test_render_cloudcfg.py | 28 ------------------------- 1 file changed, 28 deletions(-) diff --git a/tests/unittests/test_render_cloudcfg.py b/tests/unittests/test_render_cloudcfg.py index 46f4be16a6f..b8e3550d302 100644 --- a/tests/unittests/test_render_cloudcfg.py +++ b/tests/unittests/test_render_cloudcfg.py @@ -40,34 +40,6 @@ def test_variant_sets_distro_in_cloud_cfg_subp(self, tmpdir): system_cfg = util.load_yaml(stream.read()) assert system_cfg["system_info"]["distro"] == "ubuntu" - def test_variant_sets_default_user_in_cloud_cfg_subp(self, tmpdir): - outfile = tmpdir.join("outcfg").strpath - templater.render_cloudcfg( - ["--variant", "ubuntu", self.tmpl_path, outfile] - ) - with open(outfile) as stream: - system_cfg = util.load_yaml(stream.read()) - - default_user_exceptions = { - "amazon": "ec2-user", - "debian": "ubuntu", - "unknown": "ubuntu", - } - default_user = system_cfg["system_info"]["default_user"]["name"] - assert default_user == default_user_exceptions.get("ubuntu", "ubuntu") - - def test_variant_sets_network_renderer_priority_in_cloud_cfg_subp( - self, tmpdir - ): - outfile = tmpdir.join("outcfg").strpath - templater.render_cloudcfg( - ["--variant", "openbsd", self.tmpl_path, outfile] - ) - with open(outfile) as stream: - system_cfg = util.load_yaml(stream.read()) - - assert ["openbsd"] == system_cfg["system_info"]["network"]["renderers"] - @pytest.mark.parametrize("variant", (DISTRO_VARIANTS)) def test_variant_sets_distro_in_cloud_cfg(self, variant, tmpdir): """Testing parametrized inputs with imported function saves ~0.5s per From 0ed5bc7f33d9d1ac9088671c2c4b06d8e236e96d Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Jan 2022 12:36:19 -0700 Subject: [PATCH 3/5] isort --- cloudinit/templater.py | 4 ++-- tests/unittests/test_render_cloudcfg.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cloudinit/templater.py b/cloudinit/templater.py index 224fdd9e4ef..fcb9c01c941 100644 --- a/cloudinit/templater.py +++ b/cloudinit/templater.py @@ -10,10 +10,10 @@ # # This file is part of cloud-init. See LICENSE file for license information. -import sys +import argparse import collections import re -import argparse +import sys try: from Cheetah.Template import Template as CTemplate diff --git a/tests/unittests/test_render_cloudcfg.py b/tests/unittests/test_render_cloudcfg.py index b8e3550d302..a727e007fec 100644 --- a/tests/unittests/test_render_cloudcfg.py +++ b/tests/unittests/test_render_cloudcfg.py @@ -4,7 +4,7 @@ import pytest -from cloudinit import subp, util, templater +from cloudinit import subp, templater, util from tests.unittests.helpers import cloud_init_project_dir # TODO(Look to align with tools.render-cloudcfg or cloudinit.distos.OSFAMILIES) From b88cf1c14d3930ae63d1aad5d738e8f9d98b2a97 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Jan 2022 14:09:36 -0700 Subject: [PATCH 4/5] remove ugly pylint hack --- tools/render-cloudcfg | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tools/render-cloudcfg b/tools/render-cloudcfg index 93dafd3c5b2..4ae74d6bc0c 100755 --- a/tools/render-cloudcfg +++ b/tools/render-cloudcfg @@ -5,12 +5,11 @@ import sys def main(): - if "avoid-pep8-E402-import-not-top-of-file": - _tdir = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) - sys.path.insert(0, _tdir) - from cloudinit import templater + _tdir = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) + sys.path.insert(0, _tdir) + from cloudinit import templater # pylint: disable=E0401 - templater.render_cloudcfg(sys.argv[1:]) + templater.render_cloudcfg(sys.argv[1:]) if __name__ == "__main__": From 164855e8685108ba934de6f27517c82b1e47b9e5 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 13 Jan 2022 15:54:53 -0700 Subject: [PATCH 5/5] separate parsing from logic --- cloudinit/templater.py | 59 +++---------------------- tests/unittests/test_render_cloudcfg.py | 12 ++--- tools/render-cloudcfg | 53 +++++++++++++++++++++- 3 files changed, 59 insertions(+), 65 deletions(-) diff --git a/cloudinit/templater.py b/cloudinit/templater.py index fcb9c01c941..1e147d4ab64 100644 --- a/cloudinit/templater.py +++ b/cloudinit/templater.py @@ -10,7 +10,6 @@ # # This file is part of cloud-init. See LICENSE file for license information. -import argparse import collections import re import sys @@ -183,65 +182,17 @@ def render_string(content, params): return renderer(content, params) -def render_cloudcfg(argv): - VARIANTS = [ - "almalinux", - "alpine", - "amazon", - "arch", - "centos", - "cloudlinux", - "debian", - "eurolinux", - "fedora", - "freebsd", - "miraclelinux", - "netbsd", - "openbsd", - "openEuler", - "photon", - "rhel", - "suse", - "rocky", - "ubuntu", - "unknown", - "virtuozzo", - ] - parser = argparse.ArgumentParser() - platform = util.system_info() - parser.add_argument( - "--variant", - default=platform["variant"], - action="store", - help="define the variant.", - choices=VARIANTS, - ) - parser.add_argument( - "template", - nargs="?", - action="store", - default="./config/cloud.cfg.tmpl", - help="Path to the cloud.cfg template", - ) - parser.add_argument( - "output", - nargs="?", - action="store", - default="-", - help="Output file. Use '-' to write to stdout", - ) - - args = parser.parse_args(argv) +def render_cloudcfg(variant, template, output): - with open(args.template, "r") as fh: + with open(template, "r") as fh: contents = fh.read() - tpl_params = {"variant": args.variant} + tpl_params = {"variant": variant} contents = (render_string(contents, tpl_params)).rstrip() + "\n" util.load_yaml(contents) - if args.output == "-": + if output == "-": sys.stdout.write(contents) else: - write_file(args.output, contents, omode="w") + write_file(output, contents, omode="w") # vi: ts=4 expandtab diff --git a/tests/unittests/test_render_cloudcfg.py b/tests/unittests/test_render_cloudcfg.py index a727e007fec..01a728dac36 100644 --- a/tests/unittests/test_render_cloudcfg.py +++ b/tests/unittests/test_render_cloudcfg.py @@ -47,9 +47,7 @@ def test_variant_sets_distro_in_cloud_cfg(self, variant, tmpdir): """ outfile = tmpdir.join("outcfg").strpath - templater.render_cloudcfg( - ["--variant", variant, self.tmpl_path, outfile] - ) + templater.render_cloudcfg(variant, self.tmpl_path, outfile) with open(outfile) as stream: system_cfg = util.load_yaml(stream.read()) if variant == "unknown": @@ -62,9 +60,7 @@ def test_variant_sets_default_user_in_cloud_cfg(self, variant, tmpdir): call versus calling as subp """ outfile = tmpdir.join("outcfg").strpath - templater.render_cloudcfg( - ["--variant", variant, self.tmpl_path, outfile] - ) + templater.render_cloudcfg(variant, self.tmpl_path, outfile) with open(outfile) as stream: system_cfg = util.load_yaml(stream.read()) @@ -92,9 +88,7 @@ def test_variant_sets_network_renderer_priority_in_cloud_cfg( call versus calling as subp """ outfile = tmpdir.join("outcfg").strpath - templater.render_cloudcfg( - ["--variant", variant, self.tmpl_path, outfile] - ) + templater.render_cloudcfg(variant, self.tmpl_path, outfile) with open(outfile) as stream: system_cfg = util.load_yaml(stream.read()) diff --git a/tools/render-cloudcfg b/tools/render-cloudcfg index 4ae74d6bc0c..c14097b50de 100755 --- a/tools/render-cloudcfg +++ b/tools/render-cloudcfg @@ -2,14 +2,63 @@ import os import sys +import argparse def main(): _tdir = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) sys.path.insert(0, _tdir) - from cloudinit import templater # pylint: disable=E0401 + from cloudinit import templater, util # pylint: disable=E0401 - templater.render_cloudcfg(sys.argv[1:]) + VARIANTS = [ + "almalinux", + "alpine", + "amazon", + "arch", + "centos", + "cloudlinux", + "debian", + "eurolinux", + "fedora", + "freebsd", + "miraclelinux", + "netbsd", + "openbsd", + "openEuler", + "photon", + "rhel", + "suse", + "rocky", + "ubuntu", + "unknown", + "virtuozzo", + ] + parser = argparse.ArgumentParser() + platform = util.system_info() + parser.add_argument( + "--variant", + default=platform["variant"], + action="store", + help="define the variant.", + choices=VARIANTS, + ) + parser.add_argument( + "template", + nargs="?", + action="store", + default="./config/cloud.cfg.tmpl", + help="Path to the cloud.cfg template", + ) + parser.add_argument( + "output", + nargs="?", + action="store", + default="-", + help="Output file. Use '-' to write to stdout", + ) + + args = parser.parse_args(sys.argv[1:]) + templater.render_cloudcfg(args.variant, args.template, args.output) if __name__ == "__main__":