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

support extra_vars in syntax check rule #1342

Merged
merged 5 commits into from Feb 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion .ansible-lint
Expand Up @@ -38,5 +38,13 @@ warn_list:
- git-latest
- experimetal # experimental is included in the implicit list

# offline mode disables installation of requirements.yml
# Offline mode disables installation of requirements.yml
offline: false

# Define required Ansible's variables to satisfy syntax check
extra_vars:
foo: bar
multiline_string_variable: |
line1
line2
complex_variable: ":{;\t$()"
2 changes: 2 additions & 0 deletions examples/playbooks/custom_module.yml
@@ -1,5 +1,7 @@
- hosts: localhost
gather_facts: false
tags:
- "{{ foo }}"
tasks:
- name: Run custom module
fake_module: {}
9 changes: 9 additions & 0 deletions examples/playbooks/extra_vars.yml
@@ -0,0 +1,9 @@
---
- hosts: all
tags:
- baz
- "{{ foo }}"
tasks:
- name: Show `complex_variable` value loaded from `extra_vars`
ansible.builtin.debug:
msg: "{{ complex_variable }}"
1 change: 1 addition & 0 deletions src/ansiblelint/config.py
Expand Up @@ -50,6 +50,7 @@
mock_roles=[],
loop_var_prefix=None,
offline=False,
extra_vars=None,
)

# Used to store detected tag deprecations
Expand Down
24 changes: 23 additions & 1 deletion src/ansiblelint/rules/AnsibleSyntaxCheckRule.py
@@ -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
Expand Down Expand Up @@ -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)]
Copy link
Member

Choose a reason for hiding this comment

The 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 .cache/extra_vars.yml and pass @.cache/extra_vars.yml to ansible, so it will load them from there.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 shell=False below), this is passed directly as-is to ansible-playbook.

Copy link
Member

@ssbarnea ssbarnea Feb 14, 2021

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -130,3 +140,15 @@ 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',
'complex_variable': ':{;\t$()',
}
lintable = Lintable('examples/playbooks/extra_vars.yml', kind='playbook')

result = AnsibleSyntaxCheckRule._get_ansible_syntax_check_matches(lintable)

assert not result
11 changes: 11 additions & 0 deletions src/ansiblelint/testing/fixtures.py
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ConfigWrapper like for instance pytest-django do in the settings fixture (reference).
Do you have any other ideas for a more elegant solution?

Copy link
Member

Choose a reason for hiding this comment

The 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:
Expand Down