E911: Added AnsibleSyntaxCheckRule for running ansible syntax check #950

ssbarnea · 2020-08-13T08:37:39Z

Fixes issue where passing task/var files to the linter would return false-positives.
The linter reported the task files as playbooks but silently failed to lint them and passed.

From now, any argument that is not of playbook type will generate specific errors caused by ansible syntax-check fails.

This does affect performance, especially during our own testing, but the real benefit is that we now get confirmation from Ansible itself. We can look into improving performance in follow-ups if we find a way to instantiate ansible runtime only once instead of running the CLI tool every time. Still this comes with downsides too because the CLI interface is more stable. Another possible path to improve performance would be to run the syntax check async on multiple threads. Anyway, any approach will in follow-up, not as part of this initial implementation.

Fixes: #787

Updated our import_playbook tests to match ansible implementation of import_playbook and to avoid hitting ansible syntax check failures with our own testing playbooks. Needed-By: #950

Ansible requires hosts to be present in playbooks and in order to be able pass ansible syntax check we need to add it to our tests. Needed-By: #950

lib/ansiblelint/runner.py

tadeboro

I like the idea behind the proposed changes, but the implementation should be improved before this PR can be merged. Tightly coupling all sorts of unrelated stuff will not help us in the long run (we risk ending up in the same mess as with MatchError and Rule classes that now might just be the same class).

tadeboro · 2020-12-31T19:13:46Z

lib/ansiblelint/text.py

+    return re.sub(r"\x1b[^m]*m", "", data)
+
+
+def parse_ansible_syntax_check(run: CompletedProcess) -> List[MatchError]:


Why is a text-parsing function taking process as an input? It should just receive the Ansible output and parse it. Also, I am not sure if this is really a text-related function at all since it returns a list of error messages. I would expect this kind of function to be in some Ansible wrapper module.

I am totally for isolating Ansible dependency on the linter but that is not really a case of code depending on Ansible code, is almost only text processing.

But after looking a bit more at how is used, I will make it a method of the runner and keep only the stripping inside text module (others methods will be moved there in the future, once we strip more out of the utils.py kitchensink)

tadeboro · 2020-12-31T19:15:18Z

lib/ansiblelint/text.py

+def parse_ansible_syntax_check(run: CompletedProcess) -> List[MatchError]:
+    """Convert ansible syntax check output into a list of MatchError(s)."""
+    result = []
+    if run.returncode != 0:


This is useless since this function should not be called when there is nothing wrong with the input. Replacing the run input parameter with some pure text would solve this issue as a side-effect.

Is not really useless as the result of running syntax check is not a single string value, is a tuple formed of exit code, stderr and stdout content. Differences between the stream are likely to be relevant in the future, when the warnings will finally become real warnings, maybe even machine parsable if we are lucky.

The exit code is also relevant because currently value 4 means syntax check failed, 0 success and other values indicate other Ansible errors.

tadeboro

The high-level structure of changes is OK now, but there are still a few implementation details before this is ready for prime time.

tadeboro · 2021-01-01T11:43:33Z

lib/ansiblelint/runner.py

+    # https://github.com/PyCQA/pylint/issues/3240
+    # pylint: disable=unsubscriptable-object
+    CompletedProcess = subprocess.CompletedProcess[Any]
+else:
+    CompletedProcess = subprocess.CompletedProcess
+


Do we really need all this ugliness here? Type checking should make our lives easier and safer. But adding such mess to our code is doing the exact opposite by introducing heaps of code we do not need actually.

It would probably be nicer to drop all this and just use a string type annotation in the parse function declaration. I think something like this should work:

def _parse_ansible_syntax_check(run: "subprocess.CompletedProcess[Any]") -> List[MatchError]:

tadeboro · 2021-01-01T11:45:50Z

lib/ansiblelint/runner.py

+    def _parse_ansible_syntax_check(run: CompletedProcess) -> List[MatchError]:
+        """Convert ansible syntax check output into a list of MatchError(s)."""
+        result = []
+        if run.returncode != 0:


This is a no-op since the only call site makes sure this evaluates to True. We can remove either one of those two checks, but keeping both in is probably not the best way forward since we have code with the same responsibility in two places.

lib/ansiblelint/errors.py

lib/ansiblelint/runner.py

test/test_runner.py

test/using-bare-variables-success.yml

Remove rule about deprecated always_run because this will be covered by ansible syntax check, PR #950.

Remove SudoRule rule because this will be covered by ansible syntax check, PR #950.

Remove tests or parts of tests that are failing ansible-playbooks --syntax-check. We will still be able to identify these error by relying on Ansible itself once PR #950 gets merged.

Fixes: #787

tadeboro

I only found two nitpicks in the code. Feel free to ignore them and merge this PR as is.

tadeboro · 2021-01-06T17:43:06Z

lib/ansiblelint/runner.py

@@ -77,29 +77,37 @@ def is_excluded(self, file_path: str) -> bool:
    def run(self) -> List[MatchError]:
        """Execute the linting process."""
        files: List[Lintable] = list()
+        matches: List[MatchError] = list()


Nitpick: is there a reason why are we using a list here instead of a set? We do not want to have duplicates, so we might as well use set here and call matches.union instead of matches.extend.

The reason why I did kept list is because I wanted to avoid changing the signature. Anyway, I am considering more refactoring in that area and maybe adopting a container that can hide the boring parts like unique, sort and maybe even retrieving stats or subsets.

tadeboro · 2021-01-06T17:44:41Z

lib/ansiblelint/runner.py

        # update list of checked files
        self.checked_files.update([str(x.path) for x in files])

-        return sorted(matches)
+        return sorted(list(set(matches)))


Nitpick: sorted know how to handle sets (or any other iterabel), so no need to convert to list before sorting.

True, i will fix it in follow-up, I am sure that I will touch the same method very soon, before the release.

ssbarnea added the feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it. label Aug 13, 2020

ssbarnea force-pushed the feature/syntax-check branch 2 times, most recently from b494934 to 86582cd Compare August 13, 2020 14:02

ssbarnea mentioned this pull request Aug 13, 2020

Refactored import_playbook tests #951

Merged

ssbarnea added a commit that referenced this pull request Aug 13, 2020

Add missing hosts to test files

1c88bac

Ansible requires hosts to be present in playbooks and in order to be able pass ansible syntax check we need to add it to our tests. Needed-By: #950

This was referenced Aug 13, 2020

Add missing hosts to test files #952

Merged

ansible-lint not testing all files within ansible role structure #940

Closed

ssbarnea changed the title ~~WIP: Run ansible syntax check before linting playbooks~~ E911: Added AnsibleSyntaxCheckRule for running ansible syntax check Aug 13, 2020

ssbarnea added a commit that referenced this pull request Aug 13, 2020

Add missing hosts to test files (#952)

201a4e6

Ansible requires hosts to be present in playbooks and in order to be able pass ansible syntax check we need to add it to our tests. Needed-By: #950

ssbarnea force-pushed the feature/syntax-check branch from 86582cd to d633fed Compare August 13, 2020 17:01

webknjaz reviewed Aug 13, 2020

View reviewed changes

lib/ansiblelint/runner.py Outdated Show resolved Hide resolved

webknjaz reviewed Aug 13, 2020

View reviewed changes

lib/ansiblelint/runner.py Outdated Show resolved Hide resolved

ssbarnea force-pushed the feature/syntax-check branch from d633fed to c05bd06 Compare August 14, 2020 08:37

ssbarnea force-pushed the feature/syntax-check branch 2 times, most recently from 3de0d96 to 3d90e38 Compare December 31, 2020 15:06

ansible deleted a comment from webknjaz Dec 31, 2020

ssbarnea marked this pull request as ready for review December 31, 2020 15:07

ssbarnea added major Used for release notes, requires major versioning bump feature and removed feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it. labels Dec 31, 2020

ssbarnea added this to the 5.0.0 milestone Dec 31, 2020

ssbarnea requested a review from felixfontein December 31, 2020 15:09

ssbarnea force-pushed the feature/syntax-check branch 3 times, most recently from c591d50 to 0d65dcf Compare December 31, 2020 16:12

ssbarnea mentioned this pull request Dec 31, 2020

u2018 (‘) is not failing during the lint check #542

Closed

ssbarnea requested a review from tadeboro December 31, 2020 16:44

tadeboro suggested changes Dec 31, 2020

View reviewed changes

ssbarnea force-pushed the feature/syntax-check branch from 0d65dcf to 1070012 Compare January 1, 2021 10:37

ssbarnea requested review from tadeboro and webknjaz January 1, 2021 10:38

tadeboro suggested changes Jan 1, 2021

View reviewed changes

ssbarnea added a commit that referenced this pull request Jan 4, 2021

Remove always_run rule

9fb8c7a

Remove rule about deprecated always_run because this will be covered by ansible syntax check, PR #950.

ssbarnea mentioned this pull request Jan 4, 2021

Remove always_run rule #1203

Merged

ssbarnea added a commit that referenced this pull request Jan 4, 2021

Remove always_run rule (#1203)

e53cd09

Remove rule about deprecated always_run because this will be covered by ansible syntax check, PR #950.

ssbarnea added a commit that referenced this pull request Jan 4, 2021

Remove SudoRule

dc02d00

Remove SudoRule rule because this will be covered by ansible syntax check, PR #950.

ssbarnea mentioned this pull request Jan 4, 2021

Remove SudoRule #1204

Merged

ssbarnea added a commit that referenced this pull request Jan 4, 2021

Remove SudoRule (#1204)

576e709

Remove SudoRule rule because this will be covered by ansible syntax check, PR #950.

ssbarnea mentioned this pull request Jan 4, 2021

Remove tests that do not pass Ansible own syntax check #1206

Merged

ssbarnea force-pushed the feature/syntax-check branch 3 times, most recently from 1daf39e to d6dc0e1 Compare January 5, 2021 08:39

Added rule for ansible syntax check

cf93bce

Fixes: #787

ssbarnea force-pushed the feature/syntax-check branch from d6dc0e1 to cf93bce Compare January 5, 2021 14:59

ssbarnea requested a review from tadeboro January 6, 2021 09:07

tadeboro approved these changes Jan 6, 2021

View reviewed changes

ssbarnea merged commit 83436b7 into master Jan 6, 2021

ssbarnea deleted the feature/syntax-check branch January 6, 2021 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E911: Added AnsibleSyntaxCheckRule for running ansible syntax check #950

E911: Added AnsibleSyntaxCheckRule for running ansible syntax check #950

ssbarnea commented Aug 13, 2020 •

edited

Loading

tadeboro left a comment

tadeboro Dec 31, 2020

ssbarnea Jan 1, 2021

tadeboro Dec 31, 2020

ssbarnea Jan 1, 2021

tadeboro left a comment

tadeboro Jan 1, 2021

tadeboro Jan 1, 2021

tadeboro left a comment

tadeboro Jan 6, 2021

ssbarnea Jan 6, 2021

tadeboro Jan 6, 2021

ssbarnea Jan 6, 2021

		return re.sub(r"\x1b[^m]*m", "", data)


		def parse_ansible_syntax_check(run: CompletedProcess) -> List[MatchError]:

E911: Added AnsibleSyntaxCheckRule for running ansible syntax check #950

E911: Added AnsibleSyntaxCheckRule for running ansible syntax check #950

Conversation

ssbarnea commented Aug 13, 2020 • edited Loading

tadeboro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeboro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeboro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssbarnea commented Aug 13, 2020 •

edited

Loading