Jinja line statements and comments #42930

CorvinM · 2017-08-15T00:02:20Z

What does this PR do?

The jinja_line_statement_prefix and jinja_line_comment_prefix options
allow salt users to use an alternative syntax for their states and other
jinja templates.
See http://jinja.pocoo.org/docs/templates/#line-statements

What issues does this PR fix or reference?

Fixes #36942

Tests written?

No

The jinja_line_statement_prefix and jinja_line_comment_prefix options allow salt users to use an alternative syntax for their states and other jinja templates. See http://jinja.pocoo.org/docs/templates/#line-statements fixes saltstack#36942

ghost · 2017-08-15T00:02:28Z

@CorvinM, thanks for your PR! By analyzing the history of the files in this pull request, we identified @terminalmage, @thatch45 and @rallytime to be potential reviewers.

cachedout · 2017-08-15T04:15:20Z

salt/config/__init__.py

@@ -1578,6 +1586,8 @@ def _gather_buffer_space():
    'winrepo_passphrase': '',
    'winrepo_refspecs': _DFLT_REFSPECS,
    'syndic_wait': 5,
+    'jinja_line_statement_prefix': None,


This looks like a mismatch with the str type defined above in L845. Would it be better to have '' be the default for consistency?

Possibly, I went for the str & None combo because it seemed to make more sense logically to me but also because there were plenty of other options also using str & None in the same way. For example:

environment L229 & L1399

state_top_saltenv L240 & L1512

log_level_logfile L373 & L1530

etc..

But I do agree that '' should fit these two options better, plus I can remove the (hackish?) explicit check for 'none'.

Thanks, will update with '' as the default.

…tead of None

cachedout · 2017-08-15T17:42:46Z

I would like @whiteinge and @terminalmage to both take a look here for any additional feedback but I'm good with this at present.

lorengordon · 2017-08-15T19:47:52Z

May want to compare to #41538

whiteinge

Interesting Jinja feature. (New to me.) Makes the syntax less verbose, though might be tricky to maintain alongside YAML.

In any case, this looks sufficiently opt-in so 👍 . The PR that Loren pointed out has a nearly identical approach plus it also has unit tests. Both PRs should be combined IMO.

epcim · 2017-08-16T06:08:02Z

@lorengordon commented on Aug 15, 2017, 9:47 PM GMT+2:

May want to compare to #41538

#41538 - the ambition is the same as this PR +

test coverage
possibility to enable that feature only to .sls files (but not, in general, any jinja, ... templates)
possibility to configure even the other jinja options found in salt config.
and hopefully we already fine tuned all the wording in doc strings

CorvinM · 2017-08-17T01:19:31Z

Made somewhat of a merge with #41538

Note: I've ignored the documentation (to avoid having to keep rewriting it) until we come to a conclusion on implementation (or very close to a conclusion).

This commit should meet and exceed the ambitions of #41538 plus have some advantages:

Supports all the current options for the jinja environment plus any added in the future (without any code changes to salt)
Avoids fighting the yaml parser for default options (because the default is being not present in the dict)
Easily expandable to support jinja_env on a per template basis from something like file.managed
Fixes several bugs found in Extend Jinja options support (line statements) #41538

If there is any question about why I chose to do something one way over the other please ask.

@whiteinge @epcim @cachedout @terminalmage thoughts?

epcim · 2017-08-17T17:57:42Z

@CorvinM commented on Aug 17, 2017, 3:19 AM GMT+2:

Made somewhat of a merge with #41538

Note: I've ignored the documentation (to avoid having to keep rewriting it) until we come to a conclusion on implementation (or very close to a conclusion).

This commit should meet and exceed the ambitions of #41538 plus have some advantages:

Supports all the current options for the jinja environment plus any added in the future (without any code changes to salt)

Avoids fighting the yaml parser for default options (because the default is being not present in the dict)

Easily expandable to support jinja_env on a per template basis from something like file.managed

Fixes several bugs found in #41538

If there is any question about why I chose to do something one way over the other please ask.

@whiteinge @epcim @cachedout @terminalmage thoughts?

I would rather if you would send PR against Extend Jinja options support (line statements) #41538. (that one was already passing all tests). I am open to merge them as long as they have consensus here.
What bugs you found in Extend Jinja options support (line statements) #41538 ?

CorvinM · 2017-08-17T18:12:05Z

All the test failures appear to be unrelated (most of which are network errors)

CorvinM · 2017-08-17T18:12:11Z

Go Go Jenkins!

terminalmage

I like this, and I feel like the configuration is simpler than @epcim's PR. That said, I tend to agree that you should open a PR against @epcim's fork, since both of you have done a bunch of work on this, and you've reworked your PR to incorporate some of the code from @epcim's PR.

cachedout · 2017-08-28T19:47:58Z

@CorvinM We'd love to find a way forward on this. Did you see the above proposal from @terminalmage ?

CorvinM · 2017-08-29T02:05:17Z

Apologies for the slow response time, I haven't had much time to spare.

@cachedout Yes, I did see @terminalmage's proposal. I don't agree with merging @epcim 's fork but if you all want it merged (which it looks like you do) I will do it anyways. I can see the motivation even if I don't agree with it. By merge I assume the goal is to include the commits from #41538 but not the actual changes? Because, I don't really see any changes that I can merge from #41538 that would be compatible or make sense with this PR besides the test case which I already "merged" with @epcim marked as author for the commit.
As for the part about submitting a PR against #41538 I feel is out of place and is unfair to me for the amount of effort and time spent in this PR, especially since the only code that is @epcim's in this PR is the test case and is clearly marked as his.

epcim

Here are mine comments.

epcim · 2017-08-29T20:11:24Z

doc/ref/configuration/master.rst

+
+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


'' 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.

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 %%.

epcim · 2017-08-29T20:12:14Z

conf/suse/master

+# Default jinja environment options for everything but
+# salt sls files
+#jinja_env:
+#  line_statement_prefix: '#'


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

epcim · 2017-08-29T20:16:29Z

conf/suse/master

+
+# Jinja environment options for salt sls files
+#jinja_sls_env:
+#  line_statement_prefix: '#'


@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...

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 %%.

epcim · 2017-08-29T20:16:57Z

conf/master

+# Default jinja environment options for everything but
+# salt sls files
+#jinja_env:
+#  line_statement_prefix: '#'


see the comments I added to conf/suse/master

epcim · 2017-08-29T20:22:02Z

doc/ref/configuration/master.rst

+
+.. code-block:: yaml
+
+    jinja_line_statement_prefix: ''


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

epcim · 2017-08-29T20:41:12Z

@terminalmage @cachedout @CorvinM

I added some comments.
Once I read the latest version, sure I like this PR (current) more than mine - mine was mostly (minimal change / make it possible / I blindly followed current master/conf syntax approach (kind of key: value rather then key/dict).
This PR is the sophisticated approach. I want to say I don't care whether it will be merged instead of mine PR, (yes would be better if you send patch to my PR; but at the end of the day - who cares). Important is make it merged.
We should instead speak about:
- make some options enable by default and what way highlight them for the community then speak.
- do we want document default line_statement_preffix to be '' or # or %? ( I like the last one, # is not a good idea as it will trigger at any comment)
- line_statement_preffix for SLS files I suggest to be enabled by default (assuming, it don't break most of the existing formulas - can be easily tested)
Some documentation + examples should go to release notes - so people will know how to finally use it.

epcim · 2017-08-30T06:33:33Z

thanks for clarification. minor update: for comments i have mean ## is the best. as its usually already formated in editors as comment than %% which would need an syntax style update.On 30 Aug 2017 00:57, Corvin Mcpherson <notifications@github.com> wrote:@CorvinM commented on this pull request. In doc/ref/configuration/master.rst:

@@ -1925,6 +1925,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`` +--------------------- + +.. 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 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 %%. —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

cachedout · 2017-09-11T19:23:24Z

I've assigned this over to @terminalmage to have a look. He'll have a follow-up reply here shortly. Apologies for the delay.

terminalmage · 2017-09-26T12:39:19Z

doc/ref/configuration/master.rst

+.. conf_master:: jinja_line_comment_prefix
+
+``jinja_line_comment_prefix``
+---------------------


Same here with the dashed line.

terminalmage · 2017-09-26T12:39:43Z

doc/ref/configuration/master.rst

+.. conf_master:: jinja_line_statement_prefix
+
+``jinja_line_statement_prefix``
+---------------------


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.

terminalmage

Just a couple minor changes.

cachedout · 2017-09-29T11:19:59Z

@CorvinM Did you see the request for changes here from @terminalmage ?

…ation

CorvinM · 2017-10-01T21:39:14Z

@cachedout Yes, I did. Again, apologies to all for how long it has taken to get the documentation out.

@terminalmage thanks for the review, the issues should be fixed in the new docs.

@epcim I combined the two examples demonstrating the line statements and comments in your documentation into one and used it in the jinja_sls_env documentation.

epcim · 2017-11-02T15:54:10Z

So folks where we are today? Can we merge @cachedout @terminalmage

epcim · 2017-11-10T15:15:46Z

@cachedout
Quite sure you were busy with SaltConf17 recently, but can we move this to successful merge. I quite believe all requests were satisfied.

Test's were already passing, is the signing of the commit a must or you can merge anyway?

rallytime · 2017-11-10T16:08:18Z

@CorvinM GPG commit signing is not currently required, but it is encouraged for future contributions.

I've requested a re-review from @terminalmage. We can get this in, pending his review. Thanks!

terminalmage

Sorry for the delay. This looks good!

epcim and others added 5 commits July 14, 2017 14:44

Extend Jinja options support (line statements)

c770ba3

Merge branch 'develop' into pr/jinja_line_statements

0373f90

Merge branch 'develop' into pr/jinja_line_statements

87a1154

Add jinja line statement and line comment options to master config.

eb713c6

Fix jinja line statement and line comment parsing issue.

69d241c

cachedout reviewed Aug 15, 2017

View reviewed changes

Change jinja line statement and line comment prefix default to '' ins…

23e1b23

…tead of None

cachedout requested review from terminalmage and whiteinge August 15, 2017 17:40

whiteinge approved these changes Aug 15, 2017

View reviewed changes

epcim and others added 2 commits August 16, 2017 18:29

Merge unit tests from saltstack#41538

0bbb23d

Add jinja environment default options.

2dc593b

Fix jinja environment error in master.py

e2155c2

CorvinM and others added 2 commits August 17, 2017 16:18

Merge branch 'develop' into issue36942

78fd3f9

Merge branch 'develop' into issue36942

7c78a02

terminalmage reviewed Aug 23, 2017

View reviewed changes

epcim reviewed Aug 29, 2017

View reviewed changes

cachedout assigned terminalmage Sep 5, 2017

epcim mentioned this pull request Sep 20, 2017

New pillar/master_tops saltclass module #42349

Merged

Merge branch 'develop' into issue36942

7091f8e

rallytime requested a review from terminalmage September 25, 2017 17:45

terminalmage reviewed Sep 26, 2017

View reviewed changes

terminalmage suggested changes Sep 26, 2017

View reviewed changes

CorvinM added 3 commits October 1, 2017 14:36

Add jinja_env and jinja_sls_env master configuration options document…

19dcfcf

…ation

Merge branch 'pr41538' into issue36942

470265e

Fix english error in jinja_sls_env documentation

111df31

Merge branch 'develop' into issue36942

c67db52

rallytime requested a review from terminalmage October 2, 2017 00:01

terminalmage approved these changes Nov 13, 2017

View reviewed changes

terminalmage merged commit df972db into saltstack:develop Nov 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jinja line statements and comments #42930

Jinja line statements and comments #42930

CorvinM commented Aug 15, 2017

ghost commented Aug 15, 2017

cachedout Aug 15, 2017

CorvinM Aug 15, 2017

cachedout commented Aug 15, 2017

lorengordon commented Aug 15, 2017

whiteinge left a comment

epcim commented Aug 16, 2017

CorvinM commented Aug 17, 2017

epcim commented Aug 17, 2017

CorvinM commented Aug 17, 2017

CorvinM commented Aug 17, 2017

terminalmage left a comment

cachedout commented Aug 28, 2017

CorvinM commented Aug 29, 2017 •

edited

Loading

epcim left a comment

epcim Aug 29, 2017

CorvinM Aug 29, 2017

epcim Aug 29, 2017

epcim Aug 29, 2017

CorvinM Aug 31, 2017

epcim Aug 29, 2017

epcim Aug 29, 2017

epcim commented Aug 29, 2017

epcim commented Aug 30, 2017 via email

cachedout commented Sep 11, 2017

terminalmage Sep 26, 2017 •

edited

Loading

terminalmage Sep 26, 2017

terminalmage left a comment

cachedout commented Sep 29, 2017

CorvinM commented Oct 1, 2017

epcim commented Nov 2, 2017

epcim commented Nov 10, 2017

rallytime commented Nov 10, 2017

terminalmage left a comment

Jinja line statements and comments #42930

Jinja line statements and comments #42930

Conversation

CorvinM commented Aug 15, 2017

What does this PR do?

What issues does this PR fix or reference?

Tests written?

ghost commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cachedout commented Aug 15, 2017

lorengordon commented Aug 15, 2017

whiteinge left a comment

Choose a reason for hiding this comment

epcim commented Aug 16, 2017

CorvinM commented Aug 17, 2017

epcim commented Aug 17, 2017

CorvinM commented Aug 17, 2017

CorvinM commented Aug 17, 2017

terminalmage left a comment

Choose a reason for hiding this comment

cachedout commented Aug 28, 2017

CorvinM commented Aug 29, 2017 • edited Loading

epcim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epcim commented Aug 29, 2017

epcim commented Aug 30, 2017 via email

cachedout commented Sep 11, 2017

terminalmage Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terminalmage left a comment

Choose a reason for hiding this comment

cachedout commented Sep 29, 2017

CorvinM commented Oct 1, 2017

epcim commented Nov 2, 2017

epcim commented Nov 10, 2017

rallytime commented Nov 10, 2017

terminalmage left a comment

Choose a reason for hiding this comment

CorvinM commented Aug 29, 2017 •

edited

Loading

terminalmage Sep 26, 2017 •

edited

Loading