From c4c12b0ab8e6ffe60474e97e9bdf3326a21f3238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Thu, 16 Jan 2020 11:52:53 +0100 Subject: [PATCH] env.py refactoring Clarify a few variable names, make code more modular. Also, the Renderer class now makes more sense as a singleton. We took the opportunity to delete quite a lot of code. --- tests/test_env.py | 68 +++++++----- tests/test_plugins.py | 13 --- tutor/env.py | 243 ++++++++++++++++++++---------------------- tutor/fmt.py | 4 +- tutor/plugins.py | 10 -- 5 files changed, 160 insertions(+), 178 deletions(-) diff --git a/tests/test_env.py b/tests/test_env.py index fc8b9a5..15f9b39 100644 --- a/tests/test_env.py +++ b/tests/test_env.py @@ -5,6 +5,7 @@ import unittest.mock from tutor import config as tutor_config from tutor import env +from tutor import fmt from tutor import exceptions @@ -13,9 +14,17 @@ class EnvTests(unittest.TestCase): env.Renderer.reset() def test_walk_templates(self): - templates = list(env.walk_templates("local")) + renderer = env.Renderer({}, [env.TEMPLATES_ROOT]) + templates = list(renderer.walk_templates("local")) self.assertIn("local/docker-compose.yml", templates) + def test_walk_templates_partials_are_ignored(self): + template_name = "apps/openedx/settings/partials/common_all.py" + renderer = env.Renderer({}, [env.TEMPLATES_ROOT]) + templates = list(renderer.walk_templates("apps")) + self.assertIn(template_name, renderer.environment.loader.list_templates()) + self.assertNotIn(template_name, templates) + def test_pathjoin(self): self.assertEqual( "/tmp/env/target/dummy", env.pathjoin("/tmp", "target", "dummy") @@ -52,20 +61,24 @@ class EnvTests(unittest.TestCase): exceptions.TutorError, env.render_file, {}, "local", "docker-compose.yml" ) - def test_render_full(self): + def test_save_full(self): defaults = tutor_config.load_defaults() with tempfile.TemporaryDirectory() as root: - env.render_full(root, defaults) + with unittest.mock.patch.object(fmt, "STDOUT"): + env.save(root, defaults) self.assertTrue(os.path.exists(os.path.join(root, "env", "version"))) self.assertTrue( os.path.exists(os.path.join(root, "env", "local", "docker-compose.yml")) ) - def test_render_full_with_https(self): + def test_save_full_with_https(self): defaults = tutor_config.load_defaults() defaults["ACTIVATE_HTTPS"] = True with tempfile.TemporaryDirectory() as root: - env.render_full(root, defaults) + with unittest.mock.patch.object(fmt, "STDOUT"): + env.save(root, defaults) + with open(os.path.join(root, "env", "apps", "nginx", "lms.conf")) as f: + self.assertIn("ssl", f.read()) def test_patch(self): patches = {"plugin1": "abcd", "plugin2": "efgh"} @@ -88,6 +101,11 @@ class EnvTests(unittest.TestCase): def test_plugin_templates(self): with tempfile.TemporaryDirectory() as plugin_templates: + # Create plugin + plugin1 = env.plugins.DictPlugin( + {"name": "plugin1", "version": "0", "templates": plugin_templates} + ) + # Create two templates os.makedirs(os.path.join(plugin_templates, "plugin1", "apps")) with open( @@ -102,15 +120,13 @@ class EnvTests(unittest.TestCase): # Create configuration config = {"ID": "abcd"} - # Create a single plugin + # Render templates with unittest.mock.patch.object( - env.plugins, - "iter_template_roots", - return_value=[("plugin1", plugin_templates)], + env.plugins, "iter_enabled", return_value=[plugin1], ): with tempfile.TemporaryDirectory() as root: # Render plugin templates - env.save_plugin_templates("plugin1", plugin_templates, root, config) + env.save_plugin_templates(plugin1, root, config) # Check that plugin template was rendered dst_unrendered = os.path.join( @@ -126,22 +142,26 @@ class EnvTests(unittest.TestCase): def test_renderer_is_reset_on_config_change(self): with tempfile.TemporaryDirectory() as plugin_templates: + plugin1 = env.plugins.DictPlugin( + {"name": "plugin1", "version": "0", "templates": plugin_templates} + ) # Create one template - with open(os.path.join(plugin_templates, "myplugin.txt"), "w") as f: + os.makedirs(os.path.join(plugin_templates, plugin1.name)) + with open( + os.path.join(plugin_templates, plugin1.name, "myplugin.txt"), "w" + ) as f: f.write("some content") - # Load env once - config = {"PLUGINS": []} - env1 = env.Renderer.environment(config) + # Load env once + config = {"PLUGINS": []} + env1 = env.Renderer.instance(config).environment - with unittest.mock.patch.object( - env.plugins, - "iter_template_roots", - return_value=[("myplugin", plugin_templates)], - ): - # Load env a second time - config["PLUGINS"].append("myplugin") - env2 = env.Renderer.environment(config) + with unittest.mock.patch.object( + env.plugins, "iter_enabled", return_value=[plugin1], + ): + # Load env a second time + config["PLUGINS"].append("myplugin") + env2 = env.Renderer.instance(config).environment - self.assertNotIn("myplugin.txt", env1.loader.list_templates()) - self.assertIn("myplugin.txt", env2.loader.list_templates()) + self.assertNotIn("plugin1/myplugin.txt", env1.loader.list_templates()) + self.assertIn("plugin1/myplugin.txt", env2.loader.list_templates()) diff --git a/tests/test_plugins.py b/tests/test_plugins.py index 55d20ec..90d033d 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -188,19 +188,6 @@ class PluginsTests(unittest.TestCase): [("plugin1", ["myclient"])], list(plugins.iter_hooks({}, "init")) ) - def test_iter_template_roots(self): - class plugin1: - templates = "/tmp/templates" - - with unittest.mock.patch.object( - plugins.Plugins, - "iter_enabled", - return_value=[plugins.BasePlugin("plugin1", plugin1)], - ): - self.assertEqual( - [("plugin1", "/tmp/templates")], list(plugins.iter_template_roots({})) - ) - def test_plugins_are_updated_on_config_change(self): config = {"PLUGINS": []} plugins1 = plugins.Plugins(config) diff --git a/tutor/env.py b/tutor/env.py index 1cc7c93..c0623ff 100644 --- a/tutor/env.py +++ b/tutor/env.py @@ -1,7 +1,6 @@ import codecs from copy import deepcopy import os -import shutil import jinja2 import pkg_resources @@ -18,84 +17,66 @@ VERSION_FILENAME = "version" class Renderer: - ENVIRONMENT = None - ENVIRONMENT_CONFIG = None + INSTANCE = None @classmethod - def environment(cls, config): - def patch(name, separator="\n", suffix=""): - return cls.__render_patch(config, name, separator=separator, suffix=suffix) - - if cls.ENVIRONMENT_CONFIG != config: - # Load template roots + def instance(cls, config): + if cls.INSTANCE is None or cls.INSTANCE.config != config: + # Load template roots: these are required to be able to use + # {% include .. %} directives template_roots = [TEMPLATES_ROOT] - for _, plugin_template_root in plugins.iter_template_roots(config): - template_roots.append(plugin_template_root) + for plugin in plugins.iter_enabled(config): + if plugin.templates_root: + template_roots.append(plugin.templates_root) - # Create environment - environment = jinja2.Environment( - loader=jinja2.FileSystemLoader(template_roots), - undefined=jinja2.StrictUndefined, - ) - environment.filters["random_string"] = utils.random_string - environment.filters["common_domain"] = utils.common_domain - environment.filters["list_if"] = utils.list_if - environment.filters["reverse_host"] = utils.reverse_host - environment.filters["walk_templates"] = walk_templates - environment.globals["patch"] = patch - environment.globals["TUTOR_VERSION"] = __version__ - - # Update environment singleton - cls.ENVIRONMENT_CONFIG = deepcopy(config) - cls.ENVIRONMENT = environment - - return cls.ENVIRONMENT + cls.INSTANCE = cls(config, template_roots) + return cls.INSTANCE @classmethod def reset(cls): - cls.ENVIRONMENT = None - cls.ENVIRONMENT_CONFIG = None + cls.INSTANCE = None - @classmethod - def render_str(cls, config, text): - template = cls.environment(config).from_string(text) - return cls.__render(config, template) + def __init__(self, config, template_roots): + self.config = deepcopy(config) - @classmethod - def render_file(cls, config, path): - try: - template = cls.environment(config).get_template(path) - except Exception: - fmt.echo_error("Error loading template " + path) - raise - try: - return cls.__render(config, template) - except (jinja2.exceptions.TemplateError, exceptions.TutorError): - fmt.echo_error("Error rendering template " + path) - raise - except Exception: - fmt.echo_error("Unknown error rendering template " + path) - raise + # Create environment + environment = jinja2.Environment( + loader=jinja2.FileSystemLoader(template_roots), + undefined=jinja2.StrictUndefined, + ) + environment.filters["random_string"] = utils.random_string + environment.filters["common_domain"] = utils.common_domain + environment.filters["list_if"] = utils.list_if + environment.filters["reverse_host"] = utils.reverse_host + environment.filters["walk_templates"] = self.walk_templates + environment.globals["patch"] = self.patch + environment.globals["TUTOR_VERSION"] = __version__ + self.environment = environment - @classmethod - def __render(cls, config, template): - try: - return template.render(**config) - except jinja2.exceptions.UndefinedError as e: - raise exceptions.TutorError( - "Missing configuration value: {}".format(e.args[0]) - ) + def iter_templates_in(self, *path): + prefix = "/".join(path) + for template in self.environment.loader.list_templates(): + if template.startswith(prefix) and is_part_of_env(template): + yield template - @classmethod - def __render_patch(cls, config, name, separator="\n", suffix=""): + def walk_templates(self, subdir): + """ + Iterate on the template files from `templates/`. + + Yield: + path: template path relative to the template root + """ + yield from self.iter_templates_in(subdir + "/") + + def patch(self, name, separator="\n", suffix=""): """ Render calls to {{ patch("...") }} in environment templates from plugin patches. """ patches = [] - for plugin, patch in plugins.iter_patches(config, name): - patch_template = cls.environment(config).from_string(patch) + for plugin, patch in plugins.iter_patches(self.config, name): + patch_template = self.environment.from_string(patch) try: - patches.append(patch_template.render(**config)) + patches.append(patch_template.render(**self.config)) except jinja2.exceptions.UndefinedError as e: raise exceptions.TutorError( "Missing configuration value: {} in patch '{}' from plugin {}".format( @@ -107,53 +88,80 @@ class Renderer: rendered += suffix return rendered + def render_str(self, text): + template = self.environment.from_string(text) + return self.__render(template) + + def render_file(self, path): + try: + template = self.environment.get_template(path) + except Exception: + fmt.echo_error("Error loading template " + path) + raise + try: + return self.__render(template) + except (jinja2.exceptions.TemplateError, exceptions.TutorError): + fmt.echo_error("Error rendering template " + path) + raise + except Exception: + fmt.echo_error("Unknown error rendering template " + path) + raise + + def __render(self, template): + try: + return template.render(**self.config) + except jinja2.exceptions.UndefinedError as e: + raise exceptions.TutorError( + "Missing configuration value: {}".format(e.args[0]) + ) + def save(root, config): - render_full(root, config) + """ + Save the full environment, including version information. + """ + root_env = pathjoin(root) + for prefix in [ + "android/", + "apps/", + "build/", + "dev/", + "k8s/", + "local/", + "webui/", + VERSION_FILENAME, + "kustomization.yml", + ]: + save_all_from(prefix, root_env, config) + + for plugin in plugins.iter_enabled(config): + if plugin.templates_root: + save_plugin_templates(plugin, root, config) + fmt.echo_info("Environment generated in {}".format(base_dir(root))) -def render_full(root, config): - """ - Render the full environment, including version information. - """ - for subdir in ["android", "apps", "build", "dev", "k8s", "local", "webui"]: - save_subdir(subdir, root, config) - for plugin, path in plugins.iter_template_roots(config): - save_plugin_templates(plugin, path, root, config) - save_file(VERSION_FILENAME, root, config) - save_file("kustomization.yml", root, config) - - -def save_plugin_templates(plugin, plugin_path, root, config): +def save_plugin_templates(plugin, root, config): """ Save plugin templates to plugins//*. Only the "apps" and "build" subfolders are rendered. """ + plugins_root = pathjoin(root, "plugins") for subdir in ["apps", "build"]: - path = os.path.join(plugin_path, plugin, subdir) - for src in walk_templates(path, root=plugin_path): - dst = pathjoin(root, "plugins", src) - rendered = render_file(config, src) - write_to(rendered, dst) + subdir_path = os.path.join(plugin.name, subdir) + save_all_from(subdir_path, plugins_root, config) -def save_subdir(subdir, root, config): +def save_all_from(prefix, root, config): """ - Render the templates located in `subdir` and store them with the same + Render the templates that start with `prefix` and store them with the same hierarchy at `root`. """ - for path in walk_templates(subdir): - save_file(path, root, config) - - -def save_file(path, root, config): - """ - Render the template located in `path` and store it with the same hierarchy at `root`. - """ - dst = pathjoin(root, path) - rendered = render_file(config, path) - write_to(rendered, dst) + renderer = Renderer.instance(config) + for template in renderer.iter_templates_in(prefix): + rendered = renderer.render_file(template) + dst = os.path.join(root, template) + write_to(rendered, dst) def write_to(content, path): @@ -166,7 +174,7 @@ def render_file(config, *path): """ Return the rendered contents of a template. """ - return Renderer.render_file(config, os.path.join(*path)) + return Renderer.instance(config).render_file(os.path.join(*path)) def render_dict(config): @@ -202,19 +210,7 @@ def render_str(config, text): Return: substituted (str) """ - return Renderer.render_str(config, text) - - -def copy_subdir(subdir, root): - """ - Copy the templates located in `subdir` and store them with the same hierarchy - at `root`. No rendering is done here. - """ - for path in walk_templates(subdir): - src = os.path.join(TEMPLATES_ROOT, path) - dst = pathjoin(root, path) - utils.ensure_file_directory_exists(dst) - shutil.copy(src, dst) + return Renderer.instance(config).render_str(text) def check_is_up_to_date(root): @@ -256,38 +252,25 @@ def read(*path): return fi.read() -def walk_templates(subdir, root=TEMPLATES_ROOT): - """ - Iterate on the template files from `templates/`. - - Yield: - path: template path relative to the template root - """ - for dirpath, _, filenames in os.walk(template_path(subdir)): - if not is_part_of_env(dirpath): - continue - for filename in filenames: - path = os.path.join(os.path.relpath(dirpath, root), filename) - if is_part_of_env(path): - yield path - - def is_part_of_env(path): """ - Determines whether a file should be rendered or not. + Determines whether a template should be rendered or not. Note that here we don't + rely on the OS separator, as we are handling templates """ - basename = os.path.basename(path) + parts = path.split("/") + basename = parts[-1] is_excluded = False is_excluded = is_excluded or basename.startswith(".") or basename.endswith(".pyc") - is_excluded = is_excluded or basename == "__pycache__" or basename == "partials" + is_excluded = is_excluded or basename == "__pycache__" + is_excluded = is_excluded or "partials" in parts return not is_excluded -def template_path(*path): +def template_path(*path, templates_root=TEMPLATES_ROOT): """ Return the template file's absolute path. """ - return os.path.join(TEMPLATES_ROOT, *path) + return os.path.join(templates_root, *path) def data_path(root, *path): diff --git a/tutor/fmt.py b/tutor/fmt.py index 44b81ad..cc21b0a 100644 --- a/tutor/fmt.py +++ b/tutor/fmt.py @@ -1,5 +1,7 @@ import click +STDOUT = None + def title(text): indent = 8 @@ -43,4 +45,4 @@ def alert(text): def echo(text, err=False): - click.echo(text, err=err) + click.echo(text, file=STDOUT, err=err) diff --git a/tutor/plugins.py b/tutor/plugins.py index 7e558c4..51adef9 100644 --- a/tutor/plugins.py +++ b/tutor/plugins.py @@ -181,9 +181,6 @@ class Plugins: self.hooks[hook_name] = {} self.hooks[hook_name][plugin.name] = services - if plugin.templates_root: - self.template_roots[plugin.name] = plugin.templates_root - @classmethod def clear(cls): cls.INSTANCE = None @@ -222,9 +219,6 @@ class Plugins: def iter_hooks(self, hook_name): yield from self.hooks.get(hook_name, {}).items() - def iter_template_roots(self): - yield from self.template_roots.items() - def get_callable_attr(plugin, attr_name, default=None): attr = getattr(plugin, attr_name, default) @@ -272,7 +266,3 @@ def iter_patches(config, name): def iter_hooks(config, hook_name): yield from Plugins.instance(config).iter_hooks(hook_name) - - -def iter_template_roots(config): - yield from Plugins.instance(config).iter_template_roots()