Bugfix: Set loader on local copy of jinja environment

[indy-independence]

When adding the feature to specify a user defined jinja_env I think I opened up an opportunity for users to shoot themselves in the foot (I certainly did myself) by setting the env.loader in the referenced jinja_env argument instead of a local copy of the jinja_env object. If the code calling template_file uses the same jinja_env object in two threads with different FileSystemLoader paths, for example when one thread renders EOS templates from one directory and another thread renders JunOS templates from another directory, they will both update the same env.loader path causing the right templates not to be found. This could be fixed by the code calling template_file of course, but I think it's probably safer to fix with something like this patch maybe?

[dbarrosop]

Would you mind adding an example of code that reproduces the issue you describe?

[indy-independence]

Yes I have something like this:

cnaas_jinja_env = JinjaEnvironment(
    trim_blocks=True,
    lstrip_blocks=True,
    keep_trailing_newline=True
)

def blabla():
    r = task.run(task=template_file,
                 name="Generate device config",
                 template=template,
                 jinja_env=cnaas_jinja_env,
                 path=f"{local_repo_path}/{task.host.platform}",
                 **template_vars)

If the inventory that you run the task on contains both devices with task.host.platform eos and junos for example you will get errors with templates not found or including wrong templates etc.

[dbarrosop]

Mmm, I think the main issue here is supporting both passing the env and path at the same time. I think that if we send the environment we shouldn't make any modification to it and simply pass it around. So the way I see it we should change the plugin to:

    if jinja_env:
        env = jinja_env
    else:
        env = Environment(
            loader=FileSystemLoader(path), undefined=StrictUndefined, trim_blocks=True,
        )

And then, in your code you'd have something like:

def per_vendor_env(path):
   return JinjaEnvironment(
       trim_blocks=True,
       lstrip_blocks=True,
       keep_trailing_newline=True,
       loader=FileSystemLoader(path),
   )

def blabla():
    r = task.run(task=template_file,
                 name="Generate device config",
                 template=template,
                 jinja_env= per_vendor_env(f"{local_repo_path}/{task.host.platform}"),
                 **template_vars)

Thoughts?

P.S: If you cared about efficiency the function could even use a cache to create a single env per path

[indy-independence]

Yes that also makes sense to me. It might break some stuff on my end when I upgrade nornir, but as long as I patch my code before upgrading I think it should be fine :)