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

@@ -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,
+                env.plugins, "iter_enabled", return_value=[plugin1],
-                "iter_template_roots",
-                return_value=[("plugin1", plugin_templates)],
             ):
                 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)
+            env1 = env.Renderer.instance(config).environment
 
             with unittest.mock.patch.object(
-                    env.plugins,
+                env.plugins, "iter_enabled", return_value=[plugin1],
-                    "iter_template_roots",
-                    return_value=[("myplugin", plugin_templates)],
             ):
                 # Load env a second time
                 config["PLUGINS"].append("myplugin")
-                    env2 = env.Renderer.environment(config)
+                env2 = env.Renderer.instance(config).environment
 
-                self.assertNotIn("myplugin.txt", env1.loader.list_templates())
+            self.assertNotIn("plugin1/myplugin.txt", env1.loader.list_templates())
-                self.assertIn("myplugin.txt", env2.loader.list_templates())
+            self.assertIn("plugin1/myplugin.txt", env2.loader.list_templates())

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)

tutor/env.py

@@ -1,7 +1,6 @@
 import codecs
 from copy import deepcopy
 import os
-import shutil
 
 import jinja2
 import pkg_resources
@@ -18,19 +17,27 @@ VERSION_FILENAME = "version"
 
 
 class Renderer:
-    ENVIRONMENT = None
+    INSTANCE = None
-    ENVIRONMENT_CONFIG = None
 
     @classmethod
-    def environment(cls, config):
+    def instance(cls, config):
-        def patch(name, separator="\n", suffix=""):
+        if cls.INSTANCE is None or cls.INSTANCE.config != config:
-            return cls.__render_patch(config, name, separator=separator, suffix=suffix)
+            # Load template roots: these are required to be able to use
-
+            # {% include .. %} directives
-        if cls.ENVIRONMENT_CONFIG != config:
-            # Load template roots
             template_roots = [TEMPLATES_ROOT]
-            for _, plugin_template_root in plugins.iter_template_roots(config):
+            for plugin in plugins.iter_enabled(config):
-                template_roots.append(plugin_template_root)
+                if plugin.templates_root:
+                    template_roots.append(plugin.templates_root)
+
+            cls.INSTANCE = cls(config, template_roots)
+        return cls.INSTANCE
+
+    @classmethod
+    def reset(cls):
+        cls.INSTANCE = None
+
+    def __init__(self, config, template_roots):
+        self.config = deepcopy(config)
 
         # Create environment
         environment = jinja2.Environment(
@@ -41,61 +48,35 @@ class Renderer:
         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.filters["walk_templates"] = self.walk_templates
-            environment.globals["patch"] = patch
+        environment.globals["patch"] = self.patch
         environment.globals["TUTOR_VERSION"] = __version__
+        self.environment = environment
 
-            # Update environment singleton
+    def iter_templates_in(self, *path):
-            cls.ENVIRONMENT_CONFIG = deepcopy(config)
+        prefix = "/".join(path)
-            cls.ENVIRONMENT = environment
+        for template in self.environment.loader.list_templates():
+            if template.startswith(prefix) and is_part_of_env(template):
+                yield template
 
-        return cls.ENVIRONMENT
+    def walk_templates(self, subdir):
+        """
+        Iterate on the template files from `templates/<subdir>`.
 
-    @classmethod
+        Yield:
-    def reset(cls):
+            path: template path relative to the template root
-        cls.ENVIRONMENT = None
+        """
-        cls.ENVIRONMENT_CONFIG = None
+        yield from self.iter_templates_in(subdir + "/")
 
-    @classmethod
+    def patch(self, name, separator="\n", suffix=""):
-    def render_str(cls, config, text):
-        template = cls.environment(config).from_string(text)
-        return cls.__render(config, template)
-
-    @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
-
-    @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])
-            )
-
-    @classmethod
-    def __render_patch(cls, config, name, separator="\n", suffix=""):
         """
         Render calls to {{ patch("...") }} in environment templates from plugin patches.
         """
         patches = []
-        for plugin, patch in plugins.iter_patches(config, name):
+        for plugin, patch in plugins.iter_patches(self.config, name):
-            patch_template = cls.environment(config).from_string(patch)
+            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,52 +88,79 @@ 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):
+def save_plugin_templates(plugin, 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):
     """
     Save plugin templates to plugins/<plugin name>/*.
     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)
+        subdir_path = os.path.join(plugin.name, subdir)
-        for src in walk_templates(path, root=plugin_path):
+        save_all_from(subdir_path, plugins_root, config)
-            dst = pathjoin(root, "plugins", src)
-            rendered = render_file(config, src)
-            write_to(rendered, dst)
 
 
-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):
+    renderer = Renderer.instance(config)
-        save_file(path, root, config)
+    for template in renderer.iter_templates_in(prefix):
-
+        rendered = renderer.render_file(template)
-
+        dst = os.path.join(root, template)
-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)
 
 
@@ -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)
+    return Renderer.instance(config).render_str(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)
 
 
 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/<subdir>`.
-
-    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):

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)

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()