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

Add a new rule for detecting nested jinja mustache syntax #686

Merged
merged 12 commits into from Jun 16, 2020

Conversation

europ
Copy link
Contributor

@europ europ commented Feb 25, 2020

A new rule for ansible lint was added that provides nested jinja bracket pattern detection. Nesting jinja patterns can lead to failures and other problems.

\cc @amarza-rh

@lgtm-com

This comment has been minimized.

@europ europ force-pushed the nested_jinja branch 2 times, most recently from 46967ad to 25d8f95 Compare February 28, 2020 18:52
@webknjaz webknjaz closed this Mar 4, 2020
@webknjaz webknjaz reopened this Mar 4, 2020
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Overall looks okay but it's better to also add tests before merging.

@webknjaz
Copy link
Member

webknjaz commented Mar 4, 2020

recheck

@ssbarnea
Copy link
Member

recheck

@webknjaz
Copy link
Member

@europ could you please also submit tests for this feature? It'd be hard to keep it working/healthy/supported without any CI checks in place.

@europ
Copy link
Contributor Author

europ commented Mar 23, 2020

Good evening @webknjaz,

Thank you for your added patches.

I am sorry that I did not add the tests yet. I am struggling with other current tasks. I will add it soon.

Best regards.

@europ
Copy link
Contributor Author

europ commented Mar 23, 2020

📌 TODO: add tests

@europ
Copy link
Contributor Author

europ commented Mar 23, 2020

✔️ tests added

@europ
Copy link
Contributor Author

europ commented Mar 23, 2020

Hi @webknjaz, I have added the tests as soon as I could. I am sorry for the delay.

@europ europ requested a review from webknjaz March 23, 2020 21:25
@webknjaz
Copy link
Member

@europ it's alright. No need to rush. Could you please check the failing CI jobs? Looks like the tests are failing.

@europ
Copy link
Contributor Author

europ commented Mar 23, 2020

There is some warning RemovedInSphinx30Warning due to that docs build fails.

 The HTML pages are in _build/html.
/home/runner/work/ansible-lint/ansible-lint/docs/docsite/_themes/sphinx_rtd_theme/search.html:21: RemovedInSphinx30Warning: To modify script_files in the theme is deprecated. Please insert a <script> tag directly in your theme instead.
  {% endblock %}
make[1]: Leaving directory '/home/runner/work/ansible-lint/ansible-lint/docs/docsite'
ERROR: Building docs changed tracked files, include them in commit to avoid this failure.
Makefile:38: recipe for target 'htmldocs' failed
make: *** [htmldocs] Error 1
make: Leaving directory '/home/runner/work/ansible-lint/ansible-lint/docs/docsite'
ERROR: InvocationError for command /usr/bin/make -C docs/docsite htmldocs (exited with code 2)

I tried some of the shared advice I found but I didn't succeed.

I am not very familiar with jekyll / sphinx, do you have any idea how to fix this warning? It might be needed to rewrite some parts of the documentation to fix this.

@webknjaz
Copy link
Member

webknjaz commented Mar 23, 2020

I think those sphinx errors are not related to your change. But there's failures in other jobs that might be.

@webknjaz
Copy link
Member

Just to be on the safe side, rebase your branch on top of the fresh master

@webknjaz
Copy link
Member

Oh, it looks like you need to regenerate default_rules.rst:

cd ../../lib/ansiblelint; python ./generate_docs.py
../../docs/docsite/rst/rules/default_rules.rst file written
...
ERROR: Building docs changed tracked files, include them in commit to avoid this failure.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

You used Czech diacritics/letters w/ accents. They are subset of Unicode and Python 2 doesn't know what to do with them and fails expecting plain ASCII.
If you want to use Unicode in modules under Python 2, you must specify the encoding at the top so that the parser would know what to do.

==================================== ERRORS ====================================
_________________ ERROR collecting test/TestNestedJinjaRule.py _________________
.tox/ansible27/lib/python2.7/site-packages/py/_path/local.py:701: in pyimport
    __import__(modname)
E     File "/home/runner/work/ansible-lint/ansible-lint/test/TestNestedJinjaRule.py", line 1
E   SyntaxError: Non-ASCII character '\xc3' in file /home/runner/work/ansible-lint/ansible-lint/test/TestNestedJinjaRule.py on line 1, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

lib/ansiblelint/rules/NestedJinjaRule.py Show resolved Hide resolved
test/TestNestedJinjaRule.py Show resolved Hide resolved
@europ
Copy link
Contributor Author

europ commented Mar 23, 2020

BTW, should I squash the commits into one?

@webknjaz
Copy link
Member

@europ looks like test/TestRunner.py::test_runner[test/become.yml-exclude4-0] is failing. Also, I think it'd be great to use ids= arg in @pytest.mark.parametrize() because autogenerated test param names (like test/become.yml-exclude4-0) are not very informative in the test run report, in logs. Would you improve this, please?

@webknjaz
Copy link
Member

Oh, wait... You didn't touch TestRunner.py. So you don't have to fix its IDs. But it looks like your code broke that test meaning you introduced a bug that should be fixed before proceeding: https://github.com/ansible/ansible-lint/pull/686/checks?check_run_id=778145463#step:9:299.

Also, please mark your PR as "Ready for review" once the CI checks are green.

@europ
Copy link
Contributor Author

europ commented Jun 16, 2020

Hmm... TypeError: sequence item 2: expected str instance, bool found. Going to fix it. Yes, I did not touch that file TestRunner.py

def matchtask(self, file, task):

command = "".join([
value
Copy link
Member

Choose a reason for hiding this comment

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

This could be

Suggested change
value
str(value)

but also for some types it may be misleading. Like b'something' may be turned into "b'something'". So you may want to be careful here — bytes should be converted differently.

* pattern matching logic
* rule was added to docs
* unit test modification
* related docs fix
@europ europ marked this pull request as ready for review June 16, 2020 21:55
str(value)
# task properties are stored in the 'action' key
for key, value in task['action'].items()
# exclude useless values of '__file__', '__ansible_module__', '__*__', etc.
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't explain how the code works. Reading it is pretty obvious. Code comments must be used to explain why this code is here.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is not very important atm so I won't wait for the rephrase.

@webknjaz
Copy link
Member

I didn't take into account that we have a custom dynamic rule importer and the class won't match. Reverting that.

@webknjaz webknjaz changed the title nested jinja bracket rule Add a new rule for detecting nested jinja mustache syntax Jun 16, 2020
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Great job, @europ!

@webknjaz webknjaz merged commit 8a9c98e into ansible:master Jun 16, 2020
@europ
Copy link
Contributor Author

europ commented Jun 17, 2020

@webknjaz thanks for the help & review!

@europ europ deleted the nested_jinja branch June 17, 2020 07:42
@ssbarnea ssbarnea added the major Used for release notes, requires major versioning bump label Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Used for release notes, requires major versioning bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants