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

Cleanup calculation of template sls/tpl context #58238

Merged
merged 38 commits into from Oct 8, 2020

Conversation

mlasevich
Copy link
Contributor

@mlasevich mlasevich commented Aug 18, 2020

What does this PR do?

Cleanup calculation of sls* and tpl* context parameters when parsing sls files. Fix bug which under some circumstances produced wrong value for tpldir

What issues does this PR fix or reference?

Previous Behavior

See issue #56410. If directory name matched part of the path name, wrong value was calculated

New Behavior

There should be no functional difference, all the same values are being produced, the key difference is how the template path and related variables are calculated.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

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.

@mlasevich mlasevich requested a review from a team as a code owner August 18, 2020 20:05
@ghost ghost requested review from waynew and removed request for a team August 18, 2020 20:05
@sagetherage sagetherage added Magnesium Mg release after Na prior to Al Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around labels Aug 18, 2020
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I know you're already aware of the need for tests 🙃

I've added a couple of points of feedback that should help. If you need help getting some tests going on this, let me know!

salt/utils/templates.py Outdated Show resolved Hide resolved
salt/utils/templates.py Outdated Show resolved Hide resolved
@waynew waynew added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Aug 18, 2020
@mlasevich mlasevich force-pushed the feature/56410-tpldir-calculation branch from 9d7286c to 237c62b Compare August 19, 2020 05:24
@mlasevich
Copy link
Contributor Author

Been trying to satisfy the quality gates, but this is starting to spin out of control...

@mlasevich mlasevich requested a review from waynew August 19, 2020 16:15
garethgreenaway
garethgreenaway previously approved these changes Oct 6, 2020
@dwoz
Copy link
Contributor

dwoz commented Oct 6, 2020

Because we definitely want these fixes but we need to avoid breaking existing state trees I have added a feature flag for this functionality. To use these fixes add the following to your minion's config:

features:
  use_slsvars_fixes: True

This feature flag will go away in the Phosphorus (3005) and the fixed functionality will become the default.

Akm0d
Akm0d previously approved these changes Oct 6, 2020
@dwoz
Copy link
Contributor

dwoz commented Oct 6, 2020

re-run windows

@dwoz
Copy link
Contributor

dwoz commented Oct 7, 2020

re-run full all

twangboy
twangboy previously approved these changes Oct 7, 2020
@dwoz
Copy link
Contributor

dwoz commented Oct 7, 2020

re-run full all

@dwoz dwoz dismissed stale reviews from twangboy, Akm0d, and garethgreenaway via 2ff0dc8 October 7, 2020 23:06
@dwoz dwoz merged commit c35b43d into saltstack:master Oct 8, 2020
26 checks passed
@welcome
Copy link

welcome bot commented Oct 8, 2020

Congratulations on your first PR being merged! 🎉

@max-arnold
Copy link
Contributor

max-arnold commented Oct 16, 2020

I spent some time actually trying to use this feature, and it resulted in nothing but frustration:

  1. Either the docs or the code is wrong. In the docs the feature toggle is named use_slsvars_fixes, in the code and changelog it is named enable_slsvars_fixes. If you follow the docs, it won't work.
  2. There supposed to be a warning (There have been significant improvement to template variables...) that will nudge users in the right direction, but it isn't shown in the salt-call -l warning console output. Maybe it is because I installed Salt from git, maybe for some other reason...
  3. When I finally enabled it, I got this:
% sudo salt-call state.apply example_state -l warning

[CRITICAL] Rendering SLS example_state failed, render error: 'NoneType' object is not iterable
local:
    Data failed to compile:
----------
    Rendering SLS example_state failed, render error: 'NoneType' object is not iterable
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/salt/state.py", line 3959, in render_state
    state = compile_template(
  File "/usr/local/lib/python3.8/dist-packages/salt/template.py", line 101, in compile_template
    ret = render(input_data, saltenv, sls, **render_kwargs)
  File "/usr/local/lib/python3.8/dist-packages/salt/renderers/jinja.py", line 66, in render
    tmp_data = salt.utils.templates.JINJA(
  File "/usr/local/lib/python3.8/dist-packages/salt/utils/templates.py", line 234, in render_tmpl
    context.update(sls_context)
TypeError: 'NoneType' object is not iterable

The example state looks like this (it doesn't matter if it is placed in the root or in a subfolder):

{% do salt.log.warning('VAR tpldir="{}"'.format(tpldir)) %}
{% do salt.log.warning('VAR slspath="{}"'.format(slspath)) %}
{% do salt.log.warning('VAR tplfile="{}"'.format(tplfile)) %}
{% do salt.log.warning('VAR tpldot="{}"'.format(tpldot)) %}
  1. Why we need another feature toggle subsystem?

We already have the (badly named) use_superseded config namespace for that (and a decorator that can automatically switch implementations without introducing new conditionals in the code). Maybe it is better to give it a more intuitive name alias and avoid introducing another one (features)?

And (unlike use_superseded) this new features configuration namespace can't be set through pillar or grains and requires configuring and restarting every minion. It adds a lot of friction for this new feature to be adopted by users.

  1. Have these complications been considered/tested with this PR?

Maybe it worth to delay this feature until they are fixed too? Otherwise, when/if they got fixed later, we'll need another feature flag (enable_slsvars_fixes_iteration2?) and this will be even more complicated for end users...

[ADDED LATER]:

  1. How the PR testing/review process could be improved to avoid issues like this one?

There is a requirement for PRs to have unit tests. What was tested in this specific case? Why a unit test didn't trigger the crash? Why the incomplete test coverage wasn't caught during the review phase? What about actually running the code and examples from the documentation?

I understand that these questions are rhetorical at this point, but just can't ignore the problem. After all, there is SEP-10 and some expectations about code quality it should bring...

@mlasevich
Copy link
Contributor Author

Can't speak to the feature flags, that was not part of my original PR and was added by @dwoz as a compromise to get things merged "safely" to not affect existing functionality. Unfortunately the issue you see in #3 is a direct result of a bug that was apparently introduced with the feature flags - a simple, but devastating bug. When the old implementation was re-introduced in 9380b56, a bug was added that completely broke the new implementation. It is a simple enough fix(missing return statement) - I will open a new PR to fix it. :-(

As for your last point, as the names of those variables suggest, they are designed with SLS files in mind - so mapping them to non-sls files may not produce what you would expect. That said, remember that you are loading those files from an sls file, and as such, passing in the sls context into it - so you can probably pass in whatever context items you want. I will glance at it when I have a sec. FWIW, we moved away from using map.jinja files in general, as there are much better ( at least for us), ways to handle those needs.

@max-arnold
Copy link
Contributor

Thanks for the quick response! Hopefully the fix will be merged before the Magnesium is out of the doors 🤞

I also Cc'ed @myii, maybe he will provide some additional feedback on these changes after running the saltstack-formulas test suite

Copy link
Contributor

@myii myii left a comment

Choose a reason for hiding this comment

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

@mlasevich When providing the fix you've been discussing with @max-arnold, please consider these items as well.

@@ -0,0 +1,4 @@
Added features config option for feature flags. Added a feature flag
`enable_slsvars_fixes` to enable fixes to tpldir, tplfile and sls_path.
This flag will be depricated in the Phosphorus release when this functionality
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo:

Suggested change
This flag will be depricated in the Phosphorus release when this functionality
This flag will be deprecated in the Phosphorus release when this functionality

@@ -2,6 +2,21 @@
SLS Template Variable Reference
===============================


.. warning::
In the 3002 release sls_path, tplfile, and tpldir have had some significate
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo mainly, formatting for consistency:

Suggested change
In the 3002 release sls_path, tplfile, and tpldir have had some significate
In the 3002 release ``sls_path``, ``tplfile``, and ``tpldir`` have had some significant


Full path to sls template file being process on local disk. This is usually
pointing to a copy of the sls file in a cache directory. This will be in OS
specific format (windows vs posix). (It is probably best not to use this.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
specific format (windows vs posix). (It is probably best not to use this.)
specific format (Windows vs POSIX). (It is probably best not to use this.)

)

def test_generate_sls_context__one_level_init_implicit(self):
""" generate_sls_context - Basic one level with impliocit init.sls """
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo:

Suggested change
""" generate_sls_context - Basic one level with impliocit init.sls """
""" generate_sls_context - Basic one level with implicit init.sls """

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Magnesium Mg release after Na prior to Al severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
9 participants