Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Jinja imports with relative paths. Issue #13889 #47490

Merged
merged 16 commits into from Jun 8, 2018

Conversation

@plastikos
Copy link
Contributor

@plastikos plastikos commented May 4, 2018

Jinja imports will be interpreted as originating from the top of each
of the directories in the searchpath when the template name does not
begin with './' or '../'. When a template name begins with './' or
'../' then the import will be relative to the importing file.

Prior practices required the following construct:

{% from tpldir ~ '/foo' import bar %}

This patch supports a more "natural" construct:

{% from './foo' import bar %}

Comparatively when importing from a parent directory - prior practice:

{% from tpldir ~ '/../foo' import bar %}

Construct supported by this patch:

{% from '../foo' import bar %}

What does this PR do?

Supports relative Jinja imports.

What issues does this PR fix or reference?

#13889

Previous Behavior

Unfortunately {% from './foo' import bar %} does not work. This super-secret technique was required: {% from tpldir ~ '/foo' import bar %}

New Behavior

This works: {% from './foo' import bar %}

Tests written?

Yes

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Jinja imports will be interpreted as originating from the top of each
of the directories in the searchpath when the template name does not
begin with './' or '../'.  When a template name begins with './' or
'../' then the import will be relative to the importing file.

Prior practices required the following construct:

.. code-block:: yaml

  {% from tpldir ~ '/foo' import bar %}

This patch supports a more "natural" construct:

.. code-block:: yaml

  {% from './foo' import bar %}

Comparatively when importing from a parent directory - prior practice:

.. code-block:: yaml

  {% from tpldir ~ '/../foo' import bar %}

Construct supported by this patch:

.. code-block:: yaml

  {% from '../foo' import bar %}
Copy link
Contributor Author

@plastikos plastikos left a comment

There are a few details that should be cleaned-up before this is merged.

the importing file.
'''
# FIXME: somewhere do seprataor replacement: '\\' => '/'
Copy link
Contributor Author

@plastikos plastikos May 4, 2018

Translation of \ => / might be broken. Help me, Obi Juan, you're my only hope!

Copy link
Collaborator

@terminalmage terminalmage May 16, 2018

@dwoz @twangboy When using jinja imports on Windows, are Windows-specific path-separators supported? If so, are they the only ones supported, or are forward slashes also supported? I would assume both are supported, since they are in Python proper, but I don't want to assume without knowing for sure.

Copy link
Contributor

@twangboy twangboy May 17, 2018

I use Windows separators in my Jinja. I don't even have to escape them. Jinja apparently does some escaping for you.

Copy link
Contributor Author

@plastikos plastikos May 18, 2018

The tpldir = os.path.dirname(template).replace('\\', '/') construct was in the original code and appears to do some path translation on Windows. I'm not sure how to test it to ensure that I haven't broken the functionality. Is there a unit test?

'''
Salt-specific loader to find imported jinja files.
Jinja imports will be interpreted as originating from the top
Copy link
Contributor Author

@plastikos plastikos May 4, 2018

This import behavior needs to be in the documentation. Where is the best location for it?

@plastikos
Copy link
Contributor Author

@plastikos plastikos commented May 8, 2018

RETEST

@plastikos
Copy link
Contributor Author

@plastikos plastikos commented May 9, 2018

Help! Suggestions welcome!

@rallytime rallytime requested a review from terminalmage May 9, 2018
@@ -332,7 +332,7 @@ def parse_args(self, args=None, values=None):
os.path.basename(fpath).startswith('test_'):
self.options.name.append(fpath)
continue
self.exit(status=1, msg='\'{}\' is not a valid test module'.format(fpath))
self.exit(status=1, msg='\'{}\' is not a valid test module\n'.format(fpath))
Copy link
Collaborator

@terminalmage terminalmage May 16, 2018

Can you help me understand this change? I don't know what adding a newline here fixes.

Copy link
Contributor Author

@plastikos plastikos May 18, 2018

Without the newline then the error message is not on a line by itself and the command prompt is left on the same line as the error message.

'Relative path "%s" cannot be resolved without an environment',
template
)
print('Relative path "{0}" cannot be resolved without an environment'.format(template))
Copy link
Collaborator

@terminalmage terminalmage May 16, 2018

We should not be printing to the CLI here.

Copy link
Contributor Author

@plastikos plastikos May 18, 2018

Well that's embarrassing - that should have been removed.

if _template.split('/', 1)[0] == '..':
log.warning(
'Discarded template path "%s": attempts to'
' ascend outside of file roots', template
Copy link
Collaborator

@terminalmage terminalmage May 16, 2018

Since we have multiple backends, we can't assume people will be using file_roots, so this log message should reference salt:// instead.

Copy link
Contributor Author

@plastikos plastikos May 18, 2018

Okay.

the importing file.
'''
# FIXME: somewhere do seprataor replacement: '\\' => '/'
Copy link
Collaborator

@terminalmage terminalmage May 16, 2018

@dwoz @twangboy When using jinja imports on Windows, are Windows-specific path-separators supported? If so, are they the only ones supported, or are forward slashes also supported? I would assume both are supported, since they are in Python proper, but I don't want to assume without knowing for sure.

@plastikos
Copy link
Contributor Author

@plastikos plastikos commented May 19, 2018

@terminalmage - review changes, please.

@rallytime rallytime requested review from terminalmage and twangboy May 21, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Jun 6, 2018

@plastikos Is this ready to go from your side?

If so, can you also add a mention of this addition to the Fluorine release notes?

@plastikos
Copy link
Contributor Author

@plastikos plastikos commented Jun 8, 2018

@rallytime - I'll add the mention in the release notes and it will be ready to go.

@plastikos
Copy link
Contributor Author

@plastikos plastikos commented Jun 8, 2018

@rallytime , thanks for the fix-ups.

@rallytime rallytime merged commit 160cdb2 into saltstack:develop Jun 8, 2018
5 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants