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

Jinja line statements and comments #42930

Merged
merged 17 commits into from
Nov 13, 2017
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions conf/master
Original file line number Diff line number Diff line change
Expand Up @@ -566,18 +566,24 @@
# The renderer to use on the minions to render the state data
#renderer: yaml_jinja

# The Jinja renderer can strip extra carriage returns and whitespace
# See http://jinja.pocoo.org/docs/api/#high-level-api
#
# If this is set to True the first newline after a Jinja block is removed
# (block, not variable tag!). Defaults to False, corresponds to the Jinja
# environment init variable "trim_blocks".
#jinja_trim_blocks: False
#
# If this is set to True leading spaces and tabs are stripped from the start
# of a line to a block. Defaults to False, corresponds to the Jinja
# environment init variable "lstrip_blocks".
#jinja_lstrip_blocks: False
# TODO documentation
# Ignoring documentation for now
# (until agreement on implementation has been reached)

# Default jinja environment options for everything but
# salt sls files
#jinja_env:
# line_statement_prefix: '#'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the comments I added to conf/suse/master

# line_comment_prefix: '##'
# trim_blocks: True
# lstrip_blocks: True

# Jinja environment options for salt sls files
#jinja_sls_env:
# line_statement_prefix: '#'
# line_comment_prefix: '##'
# trim_blocks: True
# lstrip_blocks: True

# The failhard option tells the minions to stop immediately after the first
# failure detected in the state execution, defaults to False
Expand Down
30 changes: 18 additions & 12 deletions conf/suse/master
Original file line number Diff line number Diff line change
Expand Up @@ -537,18 +537,24 @@ syndic_user: salt
# The renderer to use on the minions to render the state data
#renderer: yaml_jinja

# The Jinja renderer can strip extra carriage returns and whitespace
# See http://jinja.pocoo.org/docs/api/#high-level-api
#
# If this is set to True the first newline after a Jinja block is removed
# (block, not variable tag!). Defaults to False, corresponds to the Jinja
# environment init variable "trim_blocks".
#jinja_trim_blocks: False
#
# If this is set to True leading spaces and tabs are stripped from the start
# of a line to a block. Defaults to False, corresponds to the Jinja
# environment init variable "lstrip_blocks".
#jinja_lstrip_blocks: False
# TODO documentation
# Ignoring documentation for now
# (until agreement on implementation has been reached)

# Default jinja environment options for everything but
# salt sls files
#jinja_env:
# line_statement_prefix: '#'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this synced with the documentation below.
I vote for % rather than # or ''

# line_comment_prefix: '##'
# trim_blocks: True
# lstrip_blocks: True

# Jinja environment options for salt sls files
#jinja_sls_env:
# line_statement_prefix: '#'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@terminalmage for SLS files I would like to see the line_statemet_prefix enabled by default. I know you wrote few months ago - now way. But I still think it would be nice new "standard" that improves SLS readability.

I quite belive it should not affect old setups. But probably we should still check, test how frequent is (ie: %) in SLS of current formulas and that for example line_statement_prefix: '%' dont affect lines if it's not the first char on the line, etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this would be nice to have enabled just for salt's sls files by default if possible. As far as my reading and testing has found there are extremely limited situations in yaml where % is allowed at the beginning of a line. But if extremely limited is still too much we could use something like %% instead, I've been unable to find any conflicts at all with %%.

# line_comment_prefix: '##'
# trim_blocks: True
# lstrip_blocks: True

# The failhard option tells the minions to stop immediately after the first
# failure detected in the state execution, defaults to False
Expand Down
36 changes: 36 additions & 0 deletions doc/ref/configuration/master.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1941,6 +1941,42 @@ the cloud profile or master config file, no templating will be performed.

userdata_template: jinja

.. conf_master:: jinja_line_statement_prefix

``jinja_line_statement_prefix``
---------------------
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extend the dashed line to match the length of the line above. Using different lengths won't make docs builds fail, but it will muddy the output with a warning.


.. versionadded:: Oxygen

Default: ``''``

If this is set to a string that is not empty, any line starting with this prefix
will be interpreted as a line statement by the jinja renderer.
Defaults to ``''`` and corresponds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'' dont corespond with and commented variant above "#"
also I don't find '' anyway usefull thant "#". Instead of I would suggest % as the char. is there anything agains % or %% ? I think it would be more close to today practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the documentation (including this file) is outdated with the code and I will likely have to rewrite most of it. The # and ## suggestions were just pulled from the example in the jinja documentation but I agree that % and %% would be much better for sls files. The only conflict I see is that % is used for yaml directives but I don't think they are used often (if at all) in salt's sls files. Will change the suggestions to use % and %%.

to the Jinja environment init variable ``line_statement_prefix``.

.. code-block:: yaml

jinja_line_statement_prefix: ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see here example from my PR.
(it took me some time to find what really works - not not everything works)


.. conf_master:: jinja_line_comment_prefix

``jinja_line_comment_prefix``
---------------------
Copy link
Contributor

@terminalmage terminalmage Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here with the dashed line.


.. versionadded:: Oxygen

Default: ``''``

If this is set to a string that is not empty, any line starting with this prefix
will be interpreted as a line comment by the jinja renderer.
Defaults to ``''`` and corresponds
to the Jinja environment init variable ``line_comment_prefix``.

.. code-block:: yaml

jinja_line_comment_prefix: ''

.. conf_master:: jinja_trim_blocks

``jinja_trim_blocks``
Expand Down
10 changes: 10 additions & 0 deletions salt/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -878,11 +878,19 @@ def _gather_buffer_space():
# check in with their lists of expected minions before giving up
'syndic_wait': int,

# TODO documentation
'jinja_env': dict,

# TODO documentation
'jinja_sls_env': dict,

# If this is set to True leading spaces and tabs are stripped from the start
# of a line to a block.
# TODO deprecated
'jinja_lstrip_blocks': bool,

# If this is set to True the first newline after a Jinja block is removed
# TODO deprecated
'jinja_trim_blocks': bool,

# Cache minion ID to file
Expand Down Expand Up @@ -1627,6 +1635,8 @@ def _gather_buffer_space():
'winrepo_passphrase': '',
'winrepo_refspecs': _DFLT_REFSPECS,
'syndic_wait': 5,
'jinja_env': {},
'jinja_sls_env': {},
'jinja_lstrip_blocks': False,
'jinja_trim_blocks': False,
'tcp_keepalive': True,
Expand Down
2 changes: 2 additions & 0 deletions salt/daemons/masterapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,8 @@ def _master_opts(self, load):
mopts['state_auto_order'] = self.opts['state_auto_order']
mopts['state_events'] = self.opts['state_events']
mopts['state_aggregate'] = self.opts['state_aggregate']
mopts['jinja_env'] = self.opts['jinja_env']
mopts['jinja_sls_env'] = self.opts['jinja_sls_env']
mopts['jinja_lstrip_blocks'] = self.opts['jinja_lstrip_blocks']
mopts['jinja_trim_blocks'] = self.opts['jinja_trim_blocks']
return mopts
Expand Down
2 changes: 2 additions & 0 deletions salt/master.py
Original file line number Diff line number Diff line change
Expand Up @@ -1146,6 +1146,8 @@ def _master_opts(self, load):
mopts[u'state_auto_order'] = self.opts[u'state_auto_order']
mopts[u'state_events'] = self.opts[u'state_events']
mopts[u'state_aggregate'] = self.opts[u'state_aggregate']
mopts[u'jinja_env'] = self.opts[u'jinja_env']
mopts[u'jinja_sls_env'] = self.opts[u'jinja_sls_env']
mopts[u'jinja_lstrip_blocks'] = self.opts[u'jinja_lstrip_blocks']
mopts[u'jinja_trim_blocks'] = self.opts[u'jinja_trim_blocks']
return mopts
Expand Down
2 changes: 2 additions & 0 deletions salt/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -2782,6 +2782,8 @@ def __gen_opts(self, opts):
opts[u'default_top'] = mopts.get(u'default_top', opts.get(u'default_top'))
opts[u'state_events'] = mopts.get(u'state_events')
opts[u'state_aggregate'] = mopts.get(u'state_aggregate', opts.get(u'state_aggregate', False))
opts[u'jinja_env'] = mopts.get(u'jinja_env', {})
opts[u'jinja_sls_env'] = mopts.get(u'jinja_sls_env', {})
opts[u'jinja_lstrip_blocks'] = mopts.get(u'jinja_lstrip_blocks', False)
opts[u'jinja_trim_blocks'] = mopts.get(u'jinja_trim_blocks', False)
return opts
Expand Down
28 changes: 26 additions & 2 deletions salt/utils/templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -314,16 +314,40 @@ def render_jinja_tmpl(tmplstr, context, tmplpath=None):
env_args['extensions'].append('jinja2.ext.loopcontrols')
env_args['extensions'].append(salt.utils.jinja.SerializerExtension)

opt_jinja_env = opts.get('jinja_env', {})
opt_jinja_sls_env = opts.get('jinja_sls_env', {})

opt_jinja_env = opt_jinja_env if isinstance(opt_jinja_env, dict) else {}
opt_jinja_sls_env = opt_jinja_sls_env if isinstance(opt_jinja_sls_env, dict) else {}

# Pass through trim_blocks and lstrip_blocks Jinja parameters
# trim_blocks removes newlines around Jinja blocks
# lstrip_blocks strips tabs and spaces from the beginning of
# line to the start of a block.
if opts.get('jinja_trim_blocks', False):
log.debug('Jinja2 trim_blocks is enabled')
env_args['trim_blocks'] = True
log.warning('jinja_trim_blocks is deprecated and will be removed in a future release, please use jinja_env and/or jinja_sls_env instead')
opt_jinja_env['trim_blocks'] = True
opt_jinja_sls_env['trim_blocks'] = True
if opts.get('jinja_lstrip_blocks', False):
log.debug('Jinja2 lstrip_blocks is enabled')
env_args['lstrip_blocks'] = True
log.warning('jinja_lstrip_blocks is deprecated and will be removed in a future release, please use jinja_env and/or jinja_sls_env instead')
opt_jinja_env['lstrip_blocks'] = True
opt_jinja_sls_env['lstrip_blocks'] = True

def opt_jinja_env_helper(opts, optname):
for k, v in six.iteritems(opts):
k = k.lower()
if hasattr(jinja2.defaults, k.upper()):
log.debug('Jinja2 environment {0} was set to {1} by {2}'.format(k, v, optname))
env_args[k] = v
else:
log.warning('Jinja2 environment {0} is not recognized'.format(k))

if 'sls' in context and context['sls'] != '':
opt_jinja_env_helper(opt_jinja_sls_env, 'jinja_sls_env')
else:
opt_jinja_env_helper(opt_jinja_env, 'jinja_env')

if opts.get('allow_undefined', False):
jinja_env = jinja2.Environment(**env_args)
Expand Down
61 changes: 61 additions & 0 deletions tests/unit/templates/test_jinja.py
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,67 @@ def test_render_with_undefined_variable_unicode(self):
)


class TestJinjaDefaultOptions(TestCase):

def __init__(self, *args, **kws):
TestCase.__init__(self, *args, **kws)
self.local_opts = {
'cachedir': TEMPLATES_DIR,
'file_client': 'local',
'file_ignore_regex': None,
'file_ignore_glob': None,
'file_roots': {
'test': [os.path.join(TEMPLATES_DIR, 'files', 'test')]
},
'pillar_roots': {
'test': [os.path.join(TEMPLATES_DIR, 'files', 'test')]
},
'fileserver_backend': ['roots'],
'hash_type': 'md5',
'extension_modules': os.path.join(
os.path.dirname(os.path.abspath(__file__)),
'extmods'),
'jinja_env': {
'line_comment_prefix': '##',
'line_statement_prefix': '%',
},
}
self.local_salt = {
'myvar': 'zero',
'mylist': [0, 1, 2, 3],
}

def test_comment_prefix(self):

template = """
%- set myvar = 'one'
## ignored comment 1
{{- myvar -}}
{%- set myvar = 'two' %} ## ignored comment 2
{{- myvar }} ## ignored comment 3
%- if myvar == 'two':
%- set myvar = 'three'
%- endif
{{- myvar -}}
"""
rendered = render_jinja_tmpl(template,
dict(opts=self.local_opts, saltenv='test', salt=self.local_salt))
self.assertEqual(rendered, u'onetwothree')

def test_statement_prefix(self):

template = """
{%- set mylist = ['1', '2', '3'] %}
%- set mylist = ['one', 'two', 'three']
%- for item in mylist:
{{- item }}
%- endfor
"""
rendered = render_jinja_tmpl(template,
dict(opts=self.local_opts, saltenv='test', salt=self.local_salt))
self.assertEqual(rendered, u'onetwothree')


class TestCustomExtensions(TestCase):

def __init__(self, *args, **kws):
Expand Down