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
support extra_vars in syntax check rule #1342
Changes from 1 commit
811ab73
06d44a6
b37f0cc
4d9208f
e421f20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
- hosts: localhost | ||
gather_facts: false | ||
tags: | ||
- "{{ foo }}" | ||
tasks: | ||
- name: Run custom module | ||
fake_module: {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
- hosts: all | ||
tags: | ||
- baz | ||
- "{{ foo }}" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
"""Rule definition for ansible syntax check.""" | ||
import json | ||
import re | ||
import subprocess | ||
import sys | ||
from typing import List | ||
|
||
from ansiblelint._internal.rules import BaseRule, RuntimeErrorRule | ||
from ansiblelint.config import options | ||
from ansiblelint.errors import MatchError | ||
from ansiblelint.file_utils import Lintable | ||
from ansiblelint.logger import timed_info | ||
|
@@ -39,7 +41,15 @@ def _get_ansible_syntax_check_matches(lintable: Lintable) -> List[MatchError]: | |
return [] | ||
|
||
with timed_info("Executing syntax check on %s", lintable.path): | ||
cmd = ['ansible-playbook', '--syntax-check', str(lintable.path)] | ||
extra_vars_cmd = [] | ||
if options.extra_vars: | ||
extra_vars_cmd = ['--extra-vars', json.dumps(options.extra_vars)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will likely fail to work well for more complex vars, like one containing shell escapable vars, multilines and so. IMHO, I would instead dump these vars inside Be sure to add some problematic var example to the list and see if it works. I really do not want to get bug reports about linter failing to load var files, when this is the job of ansible itself. Maybe we should only support mentioning the extra_vars_filename inside config and force users that want this feature to add another file with them, so we can rely on ansible to process the file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why this should fail for more complex variables. The shell is not involved in interpreting this (see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will be glad to see it as working as is, lets put something like this into the example, which also works as test-file: extra_vars:
# sample values used for testing
foo: |
line1
line2
bar: ":{;\t$()" |
||
cmd = [ | ||
'ansible-playbook', | ||
'--syntax-check', | ||
*extra_vars_cmd, | ||
str(lintable.path), | ||
] | ||
run = subprocess.run( | ||
cmd, | ||
stdin=subprocess.PIPE, | ||
|
@@ -130,3 +140,13 @@ def test_empty_playbook() -> None: | |
assert result[0].tag == "empty-playbook" | ||
assert result[0].message == "Empty playbook, nothing to do" | ||
assert len(result) == 1 | ||
|
||
def test_extra_vars_passed_to_command(config_options) -> None: | ||
"""Validate `extra-vars` are passed to syntax check command.""" | ||
config_options.extra_vars = {'foo': 'bar', 'additional_var': 'test'} | ||
print(options, config_options, options == config_options) | ||
lintable = Lintable('examples/playbooks/extra_vars.yml', kind='playbook') | ||
|
||
result = AnsibleSyntaxCheckRule._get_ansible_syntax_check_matches(lintable) | ||
|
||
assert not result |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,10 +5,12 @@ | |
|
||
pytest_plugins = ['ansiblelint.testing'] | ||
""" | ||
import copy | ||
import os | ||
|
||
import pytest | ||
|
||
from ansiblelint.config import options # noqa: F401 | ||
from ansiblelint.constants import DEFAULT_RULESDIR | ||
from ansiblelint.rules import RulesCollection | ||
from ansiblelint.runner import Runner | ||
|
@@ -50,6 +52,15 @@ def rule_runner(request): | |
return RunFromText(collection) | ||
|
||
|
||
@pytest.fixture | ||
def config_options(): | ||
"""Return configuration options that will be restored after testrun.""" | ||
global options # pylint: disable=global-statement | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not happy with this fixture, probably the better idea will be to create some There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that django approach is considerably better than what we have but if you want to do this, please do it as a separated pull-request after we sort the rest. |
||
original_options = copy.deepcopy(options) | ||
yield options | ||
options = original_options | ||
|
||
|
||
@pytest.fixture | ||
def _play_files(tmp_path, request): | ||
if request.param is None: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not how vars are defined in ansible, you mush use a dictionary, not a list of dictionaries for variables, somethng like