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

Allow Jinja2 whitespace control chars in vars #500

Merged
merged 4 commits into from Apr 3, 2019

Conversation

netzvieh
Copy link
Contributor

No description provided.

@netzvieh netzvieh force-pushed the allow-jinja-whitespace-control branch from 75f8302 to 31e4d4a Compare March 26, 2019 22:20
@netzvieh
Copy link
Contributor Author

Fixes #499

Signed-off-by: Sebastian Meyer <meyer@b1-systems.de>
@netzvieh netzvieh force-pushed the allow-jinja-whitespace-control branch from 31e4d4a to d5fb463 Compare March 27, 2019 14:26
Signed-off-by: Sebastian Meyer <meyer@b1-systems.de>
Signed-off-by: Sebastian Meyer <meyer@b1-systems.de>
@ssbarnea
Copy link
Member

ssbarnea commented Apr 2, 2019

@webknjaz Can you have a look at this?

@@ -13,7 +13,7 @@ class VariableHasSpacesRule(AnsibleLintRule):
tags = ['formatting']
version_added = 'v4.0.0'

bracket_regex = re.compile("{{[^{' ]|[^ '}]}}")
bracket_regex = re.compile("{{[^{' -]|[^ '}-]}}")
Copy link
Member

Choose a reason for hiding this comment

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

please use raw-string literals for regexps

Suggested change
bracket_regex = re.compile("{{[^{' -]|[^ '}-]}}")
bracket_regex = re.compile(r"{{[^{' -]|[^ '}-]}}")

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this regexp does this check. Can anyone explain it to me? It seems wrong (even the original one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand we want to match everything that's starting with {{ not followed by space or hyphen. Also we want to allow {{ '{{' }} so we need to allow single quotes after {{ aswell. Same thing for }}. I'm not sure why we wouldn't want to match {{{ or }}} though.

Maybe @awcrosby can tell us why it's needed?

Problem is, this actually allows doing {{'asdas'}} as {{' and '}} are not matched. Same for {{-asassa-}}. But the regex to capture that would be something like: ({{-(?! ))|({{'(?! ))|({{[^ '-])|([^' -]}})|((?<=[^ ])-}})|((?<=[^ ])'}}) and I'm not positive we want to do that.

I'm not sure if python does allow the regex to be split into multiple lines? Then we probably could introduce it with (# comment) to explain it. Basically like:

({{-(?! ))|(# match everything with {{- not followed by a space OR)
({{'(?! ))|(# match everything with {{' not followed by a space OR)
({{[^ '-])|(# match everything with {{ not followed by a space, single quotation mark or hyphen OR)
([^' -]}})|(# match everything with }} not preceded by a space, single quotation mark or hyphen OR)
((?<=[^ ])-}})|(# match everything with -}} not preceded by a space OR)
((?<=[^ ])'}})(# match everything with '}} not preceded by a space)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does work with \ at EOL, though I'm still not sure, we want to do that.

@webknjaz webknjaz requested a review from awcrosby April 2, 2019 14:35
Signed-off-by: Sebastian Meyer <meyer@b1-systems.de>
@netzvieh netzvieh force-pushed the allow-jinja-whitespace-control branch from c74f5fd to ab15b5a Compare April 2, 2019 17:00
@awcrosby
Copy link
Contributor

awcrosby commented Apr 3, 2019

The allowing of {{{ and }}} was added to the original rule in ansible-review before the rule was added to ansible-lint, I am not sure the purpose.

Thanks for pointing out this rule allows {{'asdas'}}, this may be ok for now as we are catching the most common bad formats, and limiting the number of false positives.

The PR looks good, I will merge this in - thanks for the contribution!

@awcrosby awcrosby merged commit 4f863c9 into ansible:master Apr 3, 2019
@netzvieh netzvieh deleted the allow-jinja-whitespace-control branch April 3, 2019 14:05
@awcrosby awcrosby added this to the 4.1.1 milestone May 7, 2019
@dayja78 dayja78 mentioned this pull request Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants