willthames/ansible-review

ansible-review's Issues

Ansible-review can't parse roles with dependices from custom modules

Hi!

In my playbook, I use role spring-boot, which depends from role maven-download, which provides module maven_download.

Ansible-review fails with error:

Couldn't parse task at roles/spring-boot/tasks/extract_artifact.yml:17 (no action detected in task. This often indicates a misspelled module name, or incorrect module path.)
{ 'maven_download': 'group_id={{ item.group_id }} artifact_id={{ item.artifact_id }} version={{ item.artifact_version }} classifier=apidoc dest={{ doc_dest }}/{{ item.artifact_id }}.zip repository_url={{ maven_repo }} username={{ maven_repo_username }} password={{ maven_repo_password }}',
  'name': 'download {{ item.artifact_id }} java package',
  'register': 'download_result'}

meta tasks not known by review script

In running a review, the following error is reported which is not correct.

ERROR: Standard "Tasks and handlers must be named" not met:
database/tasks/postgresql.yml:61: [ANSIBLE0011] All tasks should be named

The line concerned is:

• meta: flush_handlers

There may need to be special handling enabled to know this task type.

Crash on non-python file in `./library` folder

If you have any non-python file into your ./library folder, ansible-review will crash:

$ cat ./ansible/library/test.txt
test string

$ ansible-review -v ./ansible/library/test.txt

INFO: Reviewing all of Code (./ansible/library/test.txt)
INFO: Code ./ansible/library/test.txt declares standards version 0.1
Traceback (most recent call last):
  File "/usr/local/bin/ansible-review", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/main/__init__.py", line 95, in main
    errors = errors + candidate.review(options, lines)
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/__init__.py", line 72, in review
    return utils.review(self, settings, lines)
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/utils/__init__.py", line 120, in review
    result = standard.check(candidate, settings)
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/code.py", line 9, in code_passes_flake8
    lineno = int(line.split(':')[1])
ValueError: invalid literal for int() with base 10: ''

Presumably this just runs flake8 on the file, assuming it's python, doesn't trap and crashes. It's valid to write Ansible modules in languages other than python; I came across this because I have a custom module that's written as a shell script.

Make output easier to parse

I created a little script to wrap ansible-review and post-process the output. This was harder than it might have been, for two reasons:

1. ansible-review splits the output for some things over two lines, like this:

WARN: Best practice "Playbooks should not contain logic (vars, tasks, handlers)" not met:
ansible/playbook.yml:10: [EXTRA0008] tasks should not be required in a play

.. but not others. Most parsing tools/code/logic etc... is simpler line-by-line, so this makes it more awkward. I ended up simplifying my life by joining them back together using sed:

$ git ls-files ansible/. | xargs ansible-review 2>/dev/null | sed -n '/met:/ {N; s/[\r\n]/ /g; p}'

This has a (mostly happy) side effect of removing several "non-standard" (see below) messages, like this:

WARN: Couldn't classify file ansible/ansible.cfg

2. Not all "Best practice..." messages follow a standard output format. For example, most of the messages I'm interested in match this regex:

regex = r'^WARN: (.*) not met: (.*?):(.*?): (.*)$'

However, the message output in #21 doesn't:

WARN: Best practice "Same variable defined in siblings" not met:
ansible/group_vars/test.yml:Inventory is broken:
Attempted to read "/home/duncan/dev/tools/provisioning/ansible/ansible.log" as ini file:
/home/duncan/dev/tools/provisioning/ansible/ansible.log:1:
Expected key=value host variable assignment, got: 16:47:41,708

it goes off the rails after the filename, putting Inventory is broken where the line number would go - i.e. matching the regex, but putting the "wrong" values in.

I could use some extra code to detect these different messages, but in the case of couldn't classify & the #21 messages, they're basically problems with ansible-review (from my point of view), so I just filtered them out instead.

---

Here's the script, if you're interested:

"""
Post process the output from Ansible Review.

This script runs Ansible Review, captures the output, then
post-processes it into a Github formated markdown Checklist, which
it then outputs to stdout.

It uses the blessed module for output, so it has nice formatting
in the console, but is pipe aware, so you don't get control chars
in piped output.
"""

import os
import re
import subprocess

from blessed import Terminal
from operator import itemgetter

# Get the output from ansible-review, and collapse the two-line output into one line:
# when a line ends in `met:`, the next line is a continuation, so collapse into one line
# Skip group_vars - this outputs a non-standard msg about inventory file being broken
# see here: https://github.com/willthames/ansible-review/issues/21
cmd = r"git ls-files ansible/. | grep -v group_vars | xargs ansible-review 2>/dev/null | sed -n '/met:/ {N; s/[\r\n]/ /g; p}'"

try:
    output = subprocess.check_output(
        cmd,
        stderr=subprocess.STDOUT,
        shell=True
    ).split('\n')
except subprocess.CalledProcessError as e:
    output = e.output.split('\n')

t = Terminal()
output_lines = []


class WarningLine:
    regex = r'^WARN: (.*) not met: (.*?):(.*?): (.*)$'
    text, filename, line, rule = range(1, 5)


for line in output:
    # Convert the string line into an output_item dict, then add
    # it to the output_lines list.
    if line.startswith('WARN: '):
        # It's a warning item
        match = re.search(WarningLine.regex, line)
        if match and len(match.groups()) == 4:
            output_lines.append({
                'log_level': t.yellow('WARN'),
                'file': match.group(WarningLine.filename),
                'line': int(match.group(WarningLine.line)),
                'text': match.group(WarningLine.text),
                'rule': match.group(WarningLine.rule),
            })


# The output from ansible-review is sorted by filename, but it's nicer
# if we sort it by filename & line_no
sorted_output = sorted(output_lines, key=itemgetter('file', 'line'))

#
# Print the output_lines list, to the console, in markdown format
#
print(t.dim('# ') + t.bold('Ansible Review Checklist'))

curr_file = ''

for line in sorted_output:
    if line['file'] != curr_file:
        curr_file = line['file']
        print(t.dim('\n## ') + t.bold(curr_file))

    print t.dim('- []') + ' {l[log_level]}: Line: {l[line]:>3}: {l[text]}, rule: {l[rule]}'.format(l=line)

and here's an example of the console output:

which can be piped to a file, then opened in a markdown editor that understands github formatted markdown (like Abricotine) and used as a checklist:

---

I did attempt to get ansible-review to run from source, and add this as a feature (where it would probably be simpler to implement) - but couldn't figure out how to do that: #20

Inconsistent output in pre-commit hook compared to other hooks

Using ansible-review version v0.14.0rc2.
The output of ansible-review when you run the command in the pre-commit hook is different from yamllint for example. I am not sure if there even is a standard output format for hooks, but I think there should be. This allows users of hooks to filter and do some grep work if they want.
In the least, I think the output in the pre-commit hook for ansible-review is really too "wordy" and hard to read. The yamllint format (seen below in my example) is easier to read and determine what file failed, what line in the file, and what problem it was. It's clearly broken into nice columns.

Additionally, the pre-commit output does not need to tell me if the vars/main.yml does not declare a standards version. This seems like info I don't need to really know as a developer. I want to know the errors specifically.

Here is the example with my pre-commit hook using ansible-review and yamllint:

> git commit -m "Added some files to test linting"
ansible-review...........................................................Failed
hookid: ansible-review

WARN: RoleVars vars/main.yml is in a role that contains a meta/main.yml without a declared standards version. Using latest standards version 0.1
WARN: Best practice "YAML should be correctly indented" not met:
vars/main.yml:4: indentation should increase by 2 chars
WARN: Best practice "Vars should only occur once per file" not met:
vars/main.yml:3: Variable password occurs more than once
WARN: Best practice "Vars should only occur once per file" not met:
vars/main.yml:10: Variable password occurs more than once
WARN: Task tasks/main.yml is in a role that contains a meta/main.yml without a declared standards version. Using latest standards version 0.1
WARN: Best practice "Commands should be idempotent" not met:
tasks/main.yml:27: [301] Commands should not change things if nothing needs doing
WARN: Best practice "YAML should be correctly indented" not met:
tasks/main.yml:5: lines starting with '- ' should have same or less indentation than previous line (indent_list_items is False)
WARN: RoleVars vars/redhat.yml is in a role that contains a meta/main.yml without a declared standards version. Using latest standards version 0.1
WARN: Playbook .pre-commit-config.yaml does not present standards version. Using latest standards version 0.1
WARN: Best practice "YAML should be correctly indented" not met:
.pre-commit-config.yaml:4: lines starting with '- ' should have same or less indentation than previous line (indent_list_items is False)
WARN: Best practice "YAML should be correctly indented" not met:
.pre-commit-config.yaml:7: lines starting with '- ' should have same or less indentation than previous line (indent_list_items is False)
WARN: Best practice "YAML should be correctly indented" not met:
.pre-commit-config.yaml:15: lines starting with '- ' should have same or less indentation than previous line (indent_list_items is False)
WARN: Best practice "YAML should be correctly indented" not met:
.pre-commit-config.yaml:21: lines starting with '- ' should have same or less indentation than previous line (indent_list_items is False)
WARN: RoleVars vars/windows.yml is in a role that contains a meta/main.yml without a declared standards version. Using latest standards version 0.1
ERROR: Standard "Shell should only be used when essential" not met:
tasks/main.yml:27: [305] Use shell only when shell functionality is required
ERROR: Standard "Tasks and handlers must be named" not met:
tasks/main.yml:24: [502] All tasks should be named
ERROR: Standard "Tasks and handlers must be named" not met:
tasks/main.yml:27: [502] All tasks should be named
ERROR: Standard "Variable names should only use lowercase letters and underscore" not met:
tasks/main.yml:24: [CUSTOM0001] Variables must be named using only lowercase and underscores

yamllint.................................................................Failed
hookid: yamllint

vars/main.yml
  10:1      error    duplication of key "password" in mapping  (key-duplicates)
  12:1      error    too many blank lines (1 > 0)  (empty-lines)

tasks/main.yml
  28:1      error    too many blank lines (1 > 0)  (empty-lines)

vars/windows.yml
  2:81      error    line too long (85 > 80 characters)  (line-length)

Embrace parseable output to stdout

It is common to print smth parseable for the sake of pipe chaining among POSIX utils.
However, ansible-review doesn't seem to care about this:

✦ ➜ ansible-review rebuild-world.yml -q 1>/dev/null
Couldn't parse task at rebuild-world.yml:8 (no action detected in task. This often indicates a misspelled module name, or incorrect module path.

The error appears to have been in '<unicode string>': line 8, column 5, but may
be elsewhere in the file depending on the exact syntax problem.

(could not open file to display line))
{ 'eix': {'__file__': 'rebuild-world.yml', '__line__': 9, 'action': 'sync'},
  'name': 'Sync portage/layman trees and index ebuilds'}

I'm building a GitHub App integration demo, which would really benefit from not having to use heuristics and manual conditional parsing to retrieve structured result data.

Provide default standards

Could you provide default standards?

Documentation on disabling standards

Since #15 remains open, I'd like to disable the files_should_be_indented standard in my codebase. How can I do this?

Add custom ID to Standard

Hi,

would it be possible that everybody can add an custom ID to a Standard definition in standards.py? I like the idea to define IDs and use these in the review output and also in external documentations or something like this.

Regards

Python 3 support

Hi, I'm running into the following error:

$ git ls-files | xargs ansible-review
Traceback (most recent call last):
File "/usr/local/bin/ansible-review", line 11, in <module>
    sys.exit(main())
File "/usr/local/lib/python3.5/dist-packages/ansiblereview/__main__.py", line 57, in main
    for key, value in settings.__dict__.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'

This seems to be the culprit:

for key, value in settings.__dict__.iteritems():
https://github.com/willthames/ansible-review/blob/master/lib/ansiblereview/__main__.py#L57

The all-knowing Stack Overflow seems to indicate this is related to Python 3:

As you are in python3 , use dict.items() instead of dict.iteritems()
iteritems() was removed in python3, so you can't use this method anymore.
https://stackoverflow.com/a/30418498/399105

Does ansible-review support Python 3?

In case it helps, this is how I installed ansible-review:

$ sudo pip3 install ansible-review

And some other relevant info:

$ ansible-review --version
ansible-review 0.13.4
$ python --version
Python 3.5.2

Thanks!

Crash on invalid YAML file

If you point asible-review at this YAML file (or presumably any broken/invalid YAML file):

test: [one, two]
  three, four]

you get the error message form pyaml and ansible-review quits:

INFO: Playbook ./ansible/broken_yaml/broken_yaml_1.yml declares standards version 0.1
Failed to parse YAML in ./ansible/broken_yaml/broken_yaml_1.yml: while parsing a block mapping
  in "<unicode string>", line 1, column 1:
    test: [one, two]
    ^
expected <block end>, but found '<scalar>'
  in "<unicode string>", line 2, column 3:
      three, four]
      ^

Ansible-review should trap this error and emit an ERROR:... message, and continue.

Stacktrace when providing custom config file using ansible-review 0.14.0rc1

I am trying the new tagged v0.14.0rc1 and even though it seems to fix the original issue with vaulted strings, I hit an issue with providing a custom config file.

> cat .ansible-review
[rules]
lint = ./.ansible-linting/lint-rules/
standards = ./.ansible-linting

And running the tool produces...

> ansible-review -c .ansible-review vars/main.yml
Traceback (most recent call last):
  File "/usr/local/bin/ansible-review", line 10, in <module>
    sys.exit(main())
  File "/tmp/ansible-review/lib/ansiblereview/__main__.py", line 56, in main
    settings = read_config(options.configfile)
  File "/tmp/ansible-review/lib/ansiblereview/utils/__init__.py", line 163, in read_config
    indent_list_items=config.getboolean('rules', 'indent_list_items')
  File "/usr/local/Cellar/python@2/2.7.16/Frameworks/Python.framework/Versions/2.7/lib/python2.7/ConfigParser.py", line 369, in getboolean
    if v.lower() not in self._boolean_states:
AttributeError: 'bool' object has no attribute 'lower'

ansible-review import issue

I encountered the following issue when running ansible-review:

$ ansible-review
/apps/tools/pip/lib/python2.6/site-packages/cryptography/__init__.py:26: DeprecationWarning: Python 2.6 is no longer supported by the Python core team, please upgrade your Python. A future version of cryptography will drop support for Python 2.6
  DeprecationWarning
Traceback (most recent call last):
  File "/apps/tools/pip/bin/ansible-review", line 8, in <module>
    from ansiblereview.version import __version__
  File "/apps/tools/pip/lib/python2.6/site-packages/ansiblereview/__init__.py", line 6, in <module>
    import utils
  File "/apps/tools/pip/lib/python2.6/site-packages/ansiblereview/utils/__init__.py", line 12, in <module>
    import importlib
ImportError: No module named importlib

As you can guess this is on python2.6.6.

It seems that installing ansible-review via pip doesn't install importlib. Should this dependency exist?

Installing import lib via pip seems to have resolved the issue.

Warnings triggered for jinja2 variables even if code is commented

Looks like ansible-review or ansible-lint is triggering warning if var is detected among jinja2 curly braces but actual code is commented out.
This happens in .yml file, not in the template.

Example:

---
# if below variable is empty then ansible will try to download "{{some_var}}.md5"
i_like_turtles: true

Output:

WARN: Best practice "Variable uses should contain whitespace" not met:
.../defaults/main.yml:35: [EXTRA0001] Variables should have spaces after {{ and before }}

Expected output:
Should not generate WARN at all, because code is commented out.

Support for pre-commit hook `verbose` option

Currently when we use the pre-commit hook with ansible-review and set the verbose option on the hook definition in the .pre-commit-config.yaml file the ansible-review tool does not react to this option at all - the output of the tool is the same.

I would imagine with verbose: true, ansible-review will produce the WARN and ERROR messages. With verbose: false, ansible-review will only show the ERROR messages.

---

repos:
  - repo: https://github.com/willthames/ansible-review
    rev: v0.14.0rc2
    hooks:
      - id: ansible-review
        name: ansible-review
        entry: ansible-review -c .ansible-review
        verbose: true

Documentation link to standards.py does not exist

https://github.com/willthames/ansible-review/blob/master/examples/standards.py does not exist, but it is referred to from the documentation

UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 21: ordinal not in range(128)

Seeing this when trying to run ansible-review on a diff. Anyone see it before?

⇒  git diff origin branch | ansible-review -c .ansible-review.ini
Traceback (most recent call last):
  File "/usr/bin/ansible-review", line 11, in <module>
    sys.exit(main())
  File "/usr/lib/python2.7/site-packages/ansiblereview/__main__.py", line 76, in main
    candidates = get_candidates_from_diff(sys.stdin)
  File "/usr/lib/python2.7/site-packages/ansiblereview/__main__.py", line 20, in get_candidates_from_diff
    patch = unidiff.PatchSet(sys.stdin)
  File "/usr/lib/python2.7/site-packages/unidiff/patch.py", line 353, in __init__
    self._parse(data, encoding=encoding)
  File "/usr/lib/python2.7/site-packages/unidiff/patch.py", line 399, in _parse
    current_file._parse_hunk(line, diff, encoding)
  File "/usr/lib/python2.7/site-packages/unidiff/patch.py", line 267, in _parse_hunk
    hunk.append(original_line)
  File "/usr/lib/python2.7/site-packages/unidiff/patch.py", line 165, in append
    s = str(line)
  File "/usr/lib/python2.7/site-packages/unidiff/patch.py", line 58, in <lambda>
    cls.__str__ = lambda x: x.__unicode__().encode(DEFAULT_ENCODING)
  File "/usr/lib/python2.7/site-packages/unidiff/patch.py", line 86, in __str__
    return "%s%s" % (self.line_type, self.value)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 21: ordinal not in range(128)

Restricting run to certain tags?

Looking at the some of the rules e.g. https://github.com/willthames/ansible-review/blob/master/lib/ansiblereview/examples/lint-rules/MetaMainHasInfoRule.py it would appear you might be able to limit the scope of rules to run to certain tags. Is this correct and if so how do you do it?

Instructions for running from source don't work.

The instructions in the README for running from source are:

# Install dependency https://github.com/willthames/ansible-lint
git clone https://github.com/willthames/ansible-review
export PYTHONPATH=$PYTHONPATH:`pwd`/ansible-review/lib
export PATH=$PATH:`pwd`/ansible-review/bin

There's no /ansible-review/bin sub-folder, so these instructions don't work?

playbooks_should_not_contain_logic function errors when encountering empty playbook

Firstly, many thanks for this great tool.

I'm running ansible-review using the supplied example/standards.py against a role which contains some stub yml files which contain no tasks (yet).

When ansible-review hits one of these files using the supplied example/standards.py, it errors out when applying the 'playbooks_should_not_contain_logic' standard, as follows:

Traceback (most recent call last):
  File "/usr/local/bin/ansible-review", line 87, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/usr/local/bin/ansible-review", line 80, in main
    errors = errors + candidate.review(options, lines)
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/__init__.py", line 68, in review
    return utils.review(self, settings, lines)
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/utils/__init__.py", line 107, in review
    result = standard.check(candidate, settings)
  File "/home/anth/dev/ac/anthcourtney.cis-amazon-linux/tests/ansible-review/standards.py", line 104, in playbook_contains_logic
    for play in plays:
TypeError: 'NoneType' object is not iterable

I can work around this by adding a `if plays is not None:`` conditional to the function, however would be interested in your perspective on how to best address this.

Quiet mode prints warnings

When using the -q flag, warning messages are still printed. Is this the intended behavior? I would expect that only errors are shown. I'm glad to open a PR to address this but wanted to get feedback first.

Broken EXTRA0008 check

It errors out if there's tasks: in playbook, but using roles: is currently discouraged. One should use tasks: + import_role: instead. This check should be improved.

Treat import_* and include_* as tasks

Since these can be used in playbooks/roles alongside other tasks I think it makes sense to treat them as such for linting. If I add a name to an import task and run the linter TaskShouldHaveName this isn't picked up. Any thoughts on this?

Incorrect processing of YAML indentation

According to both YAML documentation and Ansible YAML syntax (http://docs.ansible.com/ansible/YAMLSyntax.html), the indentation that gets checked with the function files_should_be_indented is incorrect.
A warning is thrown if I have indentation at the - with a list of tags for example.

- name: Create kill link
  file:
    state: link
    src: /opt/uptime-agent/bin/upt_daemon.sh
    dest: /etc/rc.d/rc3.d/K1uptm_daemon
  tags:
    - uptimeconfig

WARN: Best practice "YAML should be correctly indented" not met:
roles/configure.uptimeagent/tasks/main.yml:11: lines starting with '- ' should have same or less indentation than previous line

The warning disappears when I remove the two spaces in front of the dash at the tag, as such.

- name: Create kill link
  file:
    state: link
    src: /opt/uptime-agent/bin/upt_daemon.sh
    dest: /etc/rc.d/rc3.d/K1uptm_daemon
  tags:
  - uptimeconfig

This doesn't follow Ansible YAML syntax as per the documentation, though it works apparently without any issues.

Can you advice on this, if it's intentional or not? Thanks in advance.

Kind regards,

Eric V.

Add support for reading and walking through content of a whole directory structure

Currently it isn't possible to provide one or more directories and to run through the whole directory tree to test every Ansible's file.
It would be a nice addition to be able to run it by providing directories and recursively browse them.
Filed the issue here as ansible-review seemed to be the most fitted for it, but maybe this could also be in ansible-lint.

ansible-review v0.14.0rc2 produces stacktrace when checking azure_rm.yml inventory plugin

The stacktrace happens because ansible-review attempts to access/load the inventory plugin and the plugin needs to import msrest, but this module is not inside the pre-commit environment. Perhaps this is an issue with pre-commit, but I am not exactly sure yet.

Here is the stacktrace:

ansible-review...........................................................Failed
hookid: ansible-review

WARN: Playbook Build.yml does not present standards version. Using latest standards version 0.1
WARN: Best practice "YAML should be correctly indented" not met:
Build.yml:8: lines starting with '- ' should have same or less indentation than previous line (indent_list_items is False)
Traceback (most recent call last):
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/bin/ansible-review", line 10, in <module>
    sys.exit(main())
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/__main__.py", line 102, in main
    errors = errors + candidate.review(options, lines)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/__init__.py", line 76, in review
    return utils.review(self, settings, lines)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/utils/__init__.py", line 121, in review
    result = standard.check(candidate, settings)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/groupvars.py", line 45, in same_variable_defined_in_competing_groups
    inv = _inv or InventoryManager(invfile).inventory
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/inventory.py", line 86, in __init__
    vault_password_files, vault_ids)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/inventory.py", line 23, in __init__
    sources=inventory)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansible/inventory/manager.py", line 162, in __init__
    self.parse_sources(cache=True)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansible/inventory/manager.py", line 220, in parse_sources
    parse = self.parse_source(source, cache=cache)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansible/inventory/manager.py", line 254, in parse_source
    parsed_this_one = self.parse_source(fullpath, cache=cache)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansible/inventory/manager.py", line 266, in parse_source
    for plugin in self._fetch_inventory_plugins():
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansible/inventory/manager.py", line 199, in _fetch_inventory_plugins
    plugin = inventory_loader.get(name)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansible/plugins/loader.py", line 552, in get
    self._module_cache[path] = self._load_module_source(name, path)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansible/plugins/loader.py", line 530, in _load_module_source
    module = imp.load_source(to_native(full_name), to_native(path), module_file)
  File "/home/user/.cache/pre-commit/repoN7_lSN/py_env-python2.7/lib/python2.7/site-packages/ansible/plugins/inventory/azure_rm.py", line 186, in <module>
    from msrest import ServiceClient, Serializer, Deserializer
ImportError: No module named msrest

Success.

ability to load configuration from current repository (setup.cfg)

ansible-review should be able to detect configuration rules by looking inside current code directory, just like any other build and linting tools pep8, flake8, tox.

Having a user specific config file that is not kept as the same place as the code does not work well for codebases shared across multiple users.

I think that the use of setup.cfg should be a good idea as it avoids spamming the repository root with tons of config files for each tool. Most tools already support it.

Add support for decrypting vault file

Right now, trying to run ansible-review with the default standards when a group_vars or host_vars file is encrypted using ansible-vault results in the error:

ERROR: Standard "Inventory must be parseable" not met:
inventory/hosts:Inventory is broken: Decryption failed on inventory/group_vars/all/private.yml

The vault password file directive is used in the ansible.cfg file in the playbook directory.

ansible-review should either (1) add an option to supply a vault password file or (2) pull it from the ansible config via the ansible API (not sure how difficult this is).

ansible review with git diff sometimes aborts on non yaml files, non yaml should be skipped

We use following to do ansible review only on changes :
git diff origin/develop | ansible-review -c tests/ansible-review/config.ini
But if one of the changed files is not a yaml file the program aborts.
example file content that causes problems :
{
"timestamp": {{ report_timestamp | to_nice_json }},
"version": {{ version_framework | to_nice_json }},
"report_version": {{ version_reporting | to_nice_json }},
"id": {{ asset_id | to_nice_json }},
"info": {
"lab": {{ asset_lab | to_nice_json }},
{% if "hostname" in asset_info %}
"hostname": {{ asset_info["hostname"] | to_nice_json }},
{% endif %}
{% if "interfaces" in asset_info %}
"interfaces": {{ asset_info["interfaces"] | to_nice_json }},
{% endif %}
{% if "os" in asset_info %}
"os": {{ asset_info["os"] | to_nice_json }},
{% endif %}
{% if "wsus_id" in asset_info %}
"wsus_id": {% if asset_info["wsus_id"]=="" %}null{% else %}{{ asset_info["wsus_id"] | to_nice_json }}{% endif %},
{% endif %}
{% if (asset_errors is defined) and (asset_errors | length > 0) %}
"errors": {{ asset_errors | to_nice_json }},
{% endif %}
"status": {{ asset_info["status"] | to_nice_json }}
}
}
the output is :
Failed to parse YAML in roles/report/templates/asset_info.json.j2: while scanning for the next token
found character '%' that cannot start any token
in "", line 8, column 4:
{% if "hostname" in asset_info %}
^

Bug using get_action_tasks

There is a bug using get_action_tasks. Therefor i think all rules which uses get_action_tasks from ansiblelint will not work:

The function try to get file['type'] but the corresponding candidate object from ansiblereview has no attribute named like that. There is only an attribute 'filetype'. So to get get_action_tasks working the simplest workaround is to set candidate.type = candidate.filetype before calling get_action_tasks()

Regards

Octal Permission representation

This:

- name: Template Mcrouter config
  template:
    src: mcrouter-config.json.j2
    dest: /etc/mcrouter/mcrouter-config.json
    owner: mcrouter
    group: mcrouter
    mode: 0755
  tags:
    - mcrouter-config

becomes:

{'delegate_to': None, 'name': 'Template Mcrouter config', 'tags': ['mcrouter-config'], '__file__': 'mcrouter/tasks/mcrouter.yml', '__line__': 44, '__ansible_action_type__': 'task', 'action': {'src': 'mcrouter-config.json.j2', '__ansible_arguments__': [], 'dest': '/etc/mcrouter/mcrouter-config.json', '__ansible_module__': 'template', '__file__': 'mcrouter/tasks/mcrouter.yml', '__line__': 45, 'mode': 493, 'owner': 'mcrouter', 'group': 'mcrouter'}}

I was looking to create a lint for octal permissions but their representation in ansible-review seems off.

Check for duplicate inventory variables in inheritance hierarchies

We currently check for duplicate inventory variables at the file level.

For a group_vars or host_vars or inventory change, would be good to check the whole inventory hierarchy to see if any new duplicate variables appear as a result of diamond inheritance hierarchies.

same_variables_defined_in_competing_groups fails with Ansible 2.4

When this rule runs on an inventory file, it fails with the following traceback:

Traceback (most recent call last):                                                    
  File "/home/pmacarth/.local/bin/ansible-review", line 11, in <module>
    load_entry_point('ansible-review==0.13.4', 'console_scripts', 'ansible-review')()
  File "/home/pmacarth/.local/lib/python2.7/site-packages/ansiblereview/__main__.py", line 95, in main
    errors = errors + candidate.review(options, lines)
  File "/home/pmacarth/.local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 76, in review
    return utils.review(self, settings, lines)
  File "/home/pmacarth/.local/lib/python2.7/site-packages/ansiblereview/utils/__init__.py", line 120, in review
    result = standard.check(candidate, settings)
  File "/home/pmacarth/.local/lib/python2.7/site-packages/ansiblereview/groupvars.py", line 56, in same_variable_defined_in_competing_groups
    var_manager = ansible.vars.VariableManager()
AttributeError: 'module' object has no attribute 'VariableManager'

Fixing this to import from ansible.vars.manager only makes it fail with another traceback:

Traceback (most recent call last):
  File "/home/pmacarth/.local/bin/ansible-review", line 11, in <module>
    load_entry_point('ansible-review==0.13.4', 'console_scripts', 'ansible-review')()
  File "/home/pmacarth/.local/lib/python2.7/site-packages/ansiblereview/__main__.py", line 95, in main
    errors = errors + candidate.review(options, lines)
  File "/home/pmacarth/.local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 76, in review
    return utils.review(self, settings, lines)
  File "/home/pmacarth/.local/lib/python2.7/site-packages/ansiblereview/utils/__init__.py", line 120, in review
    result = standard.check(candidate, settings)
  File "/home/pmacarth/.local/lib/python2.7/site-packages/ansiblereview/groupvars.py", line 61, in same_variable_defined_in_competing_groups
    inv = _inv or ansible.inventory.Inventory(loader=loader,
AttributeError: 'module' object has no attribute 'Inventory'

This would appear to be due to the overhaul in ansible/ansible#23001, so the corresponding code here should be adjusted to allow it to work with Ansible 2.4.

Classification of files as group_vars not working correctly

There seems to be a bug in classification of files under group_vars if group_vars contains subdirectories e. g.

group_vars
|___ group_1
|_______ main.yml
|_______ vaulted.yml
|___ group_2
...

Regards

release 0.14rc2 produces stack trace when trying to read vaulted file

We are using the latest RC (0.14rc2) and had a vaulted file vars/pass.yml and ansible-review crashed with stacktrace when it hit that file. ansible-review is being triggered through pre-commit in this case.

WARN: RoleVars vars/pass.yml is in a role that contains a meta/main.yml without a declared standards version. Using latest standards version 0.1

Traceback (most recent call last):

  File "/home/user/.cache/pre-commit/repolIoGop/py_env-python2.7/bin/ansible-review", line 10, in <module>

    sys.exit(main())

  File "/home/user/.cache/pre-commit/repolIoGop/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/__main__.py", line 102, in main

    errors = errors + candidate.review(options, lines)

  File "/home/user/.cache/pre-commit/repolIoGop/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/__init__.py", line 76, in review

    return utils.review(self, settings, lines)

  File "/home/user/.cache/pre-commit/repolIoGop/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/utils/__init__.py", line 121, in review

    result = standard.check(candidate, settings)

  File "/home/user/.cache/pre-commit/repolIoGop/py_env-python2.7/lib/python2.7/site-packages/ansiblereview/vars.py", line 45, in repeated_vars

    for err_key in errors for err_line in errors[err_key]])

TypeError: string indices must be integers, not str

The workaround was to replace the vaulted file with a file containing encrypt_string vaulted strings.

Classify LICENCE and exclude dotfiles

For now, LICENCE Files are not classified by ansible-review. Also dotfiles (in my case .drone.yml) would be handled as a playbook so i think it would be a better solution to exclude (optionally) dotfiles from review. DO you accept pull requests?

By the way, whats the status of the project? There are Pull Requests not merged since April 2017..

Crash on python file in a `task/templates` folder

We have a task that needs to use the template module to output a python file to the target node. This tasks folder looks like this:

$ tree ./ansible/roles/test_role
./ansible/roles/test_role
├── files
│   └── other.py
├── tasks
│   ├── main.yml
├── templates
│   ├── test.py

The test.py jinja template looks like this:

DEPLOYMENT = dict(
     env='{{ test.env }}'
)

ansible-review is trying to flake8 this file and failing, leading to a crash, same as #26:

$ ansible-review -v ./ansible/roles/test/templates/test.py

INFO: Using configuration file: /home/duncan/.config/ansible-review/config.ini
INFO: Reviewing all of Code (./ansible/roles/test/templates/test.py)
INFO: Code ./ansible/roles/test/templates/test.py declares standards version 0.1
Traceback (most recent call last):
  File "/usr/local/bin/ansible-review", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/main/__init__.py", line 95, in main
    errors = errors + candidate.review(options, lines)
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/__init__.py", line 72, in review
    return utils.review(self, settings, lines)
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/utils/__init__.py", line 120, in review
    result = standard.check(candidate, settings)
  File "/usr/local/lib/python2.7/dist-packages/ansiblereview/code.py", line 9, in code_passes_flake8
    lineno = int(line.split(':')[1])
ValueError: invalid literal for int() with base 10: ''

Make unversioned check failures more understandable

There are three situations when a check fails against a playbook (unversioned playbooks should be treated as playbooks on very latest version)

1. check version > playbook version. Should warn that the playbook doesn't pass a "Future Standard"
2. check version <= playbook version. Should error that the playbook doesn't pass a Standard
3. unversioned check. Should warn that the playbook doesn't pass a "Best practice"

AttributeError: 'InventoryManager' object has no attribute 'get_group'

We have a script that calls review(), and this crashes in same_variable_defined_in_competing_groups(), because we call get_group() on InventoryManager, and this method does not exist.

$ python $(git rev-parse --show-toplevel)/test/run-review.py
Traceback (most recent call last):
  File "/home/kdreyer/dev/plm-playbooks/test/run-review.py", line 121, in <module>
    errors = errors + arg.review(settings, lines)
  File "/usr/lib/python2.7/site-packages/ansiblereview/__init__.py", line 76, in review
    return utils.review(self, settings, lines)
  File "/usr/lib/python2.7/site-packages/ansiblereview/utils/__init__.py", line 120, in review
    result = standard.check(candidate, settings)
  File "/usr/lib/python2.7/site-packages/ansiblereview/groupvars.py", line 76, in same_variable_defined_in_competing_groups
    group = inv.get_group(os.path.basename(candidate.path))
AttributeError: 'InventoryManager' object has no attribute 'get_group'

I tried downgrading ansible a few times, and it's the same result for each of these:
ansible-2.6.4-1.fc28
ansible-2.5.5-2.fc28
ansible-2.4.3.0-2.fc28

document ansible-review vs ansible-lint

I think that the relationship between ansible-review and ansible-lint should be explained in the main README, making clear when you may/should want one use or another.

Warnings vs Errors

Is there a way to define rules as just warnings instead of an error? Meaning, the rule will be listed when running ansible-review but it won't produce a return code of 1, which prevents the git commit. I see this feature on yamllint tool and would love to be able to do that here too. Thanks!

Not detecting malformed blocks

example main.yaml:

- when: ansible_distribution_release == 'xenial'
  block:

output - no error.
ansible shows somethig way different:

ERROR! A malformed block was encountered.

The error appears to have been in 'roles/something/tasks/main.yml': line ...., column 3, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:


- when: ansible_distribution_release == 'xenial'
  ^ here

Expected behavior: detect and report malformed blocks.

"Inventory is broken" message when processing `group_vars` files.

If I let ansible-review process my group_vars files, I get messages like this in the output:

WARN: Best practice "Same variable defined in siblings" not met:
ansible/group_vars/test.yml:Inventory is broken:
Attempted to read "/home/duncan/dev/tools/provisioning/ansible/ansible.log" as ini file:
/home/duncan/dev/tools/provisioning/ansible/ansible.log:1:
Expected key=value host variable assignment, got: 16:47:41,708

I'm running ansible-review like this: git ls-files ansible/. | xargs ansible-review.

Two things puzzle me about this:

1. It's complaining about ansible.log - which is .gitignored and thus not in the output from git ls-files ansible/. - so I guess ansible-review isn't using that list as a list of files to process, but reading the filesystem itself? Is there a way to make ansible-review ignore some files?
2. I'm not sure what this has to do with the group_vars, or with the inventory, for that matter? Is it assuming ansible.log is my inventory file?

If I exclude the group_vars folder, this problem goes away: git ls-files ansible/. | grep -v group_vars | xargs ansible-review - which indicates that maybe ansible-review does use the list of files?

Unable to Load Variables from Directories within defaults/main

This is a new feature with Ansible 2.6.
More information regarding it can be found here:

ansible/ansible#36357

Currently, an IOError is raised.

Traceback (most recent call last):
  File "/usr/local/bin/ansible-review", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/__main__.py", line 86, in main
    candidate = classify(filename)
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 184, in classify
    return RoleVars(filename)
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 87, in __init__
    super(RoleFile, self).__init__(filename)
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 68, in __init__
    self.version = find_version(filename)
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 231, in find_version
    with codecs.open(filename, mode='rb', encoding='utf-8') as f:
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 898, in open
    file = __builtin__.open(filename, mode, buffering)
IOError: [Errno 21] Is a directory: './roles/foreman-server/defaults/main'

Broken PyPI metadata and related problems

Look at https://pypi.org/project/ansible-review/#description, it shows:

Project description
The author of this package has not provided a project description

This probably means that you use outdated setuptools when building the dist.

There's also a few other problems I see:

1. setup.cfg refers to README.md, but RST is default for warehouse. To make it understand makrdown you need to provide extra setting with mime type specified correctly (and use up-to-date setuptools)
2. PyPI page would also benefit from project_urls metadata option
3. It's better readable when you keep metadata in declarative config. Here's example shim for making old setuptools understanding how to read that: https://github.com/cherrypy/cheroot/blob/master/setup.py
4. You publish only sdist, while per current recommendations you should also publish a universal wheel as well. This would also save end-users from having to have modern setuptools in place.
5. Version is hardcoded, alternatively it could be stored in a Git tag. There's a plugin called setuptools-scm, which helps to handle this smoothly.
6. Manual building/publishing is error-prone (because of human factor). I suggest changing this towards using CI/CD (the project is about automation, but doesn't have automation in place? hmm..)

So if you're open to such changes let me know as I've gotten a very good experience with all I mentioned above and I can send a PR or/and consult you on those if you have any questions/conserns.

Couldn't classify file ./ansible/roles

I can't get ansible-review to process my roles. It'll process my playbooks in the root, but won't look at the roles (where the bulk of the work is).

If I run this:

$ git ls-files ansible/. | grep role
ansible/roles/role_1/templates/template.sh.j2
ansible/roles/role_1/tasks/main.yml
...

I see roles as part of the output from git ls-files, as expected. However, if I then run this:

$ git ls-files ansible/. | xargs ansible-review -v | grep roles

I get nothing. However, if I run this it'll tell me it can't classify the roles:

$ ansible-review -v ./ansible/* | grep role
WARN: Couldn't classify file ./ansible/roles

$ ansible-review ./ansible/roles/*
WARN: Couldn't classify file ./ansible/roles/role_1
WARN: Couldn't classify file ./ansible/roles/role_2
...

My folder structure looks like this:

./provisioning/
└── ansible/
    ├── group_vars/
    │   ├── all.yml
    │   ├── group_1.yml
    │   └── group_2.yml
    ├── hosts/
    │   └── host1.yml
    ├── roles/
    │   ├── role_1/
    │   ├── role_2/
    │   ...
    ├── ansible.cfg
    ├── ansible.log
    ├── playbook_1.yml
    ├── playbook_2.yml
    │   ...

I'm in the provisioning folder at the top when I'm running ansible-review.

Traceback error on missing double quotes in yaml with Jinja2

Python 2.7.12
pip freeze (fragment):

ansible==2.2.1.0
ansible-lint==3.4.13
ansible-review==0.13.0

source yaml:

  - name: This code will fail because 'port' is not wrapped in double quotes
    wait_for:
      host: "{{ somevar }}"
      port: {{ i_have_missing_double_quotes }}
      timeout: "{{ i_am_ok }}"

error trace:

censored/roles/censored/tasks/main.yml:29: Task arguments appear to be in key value rather than YAML format
Traceback (most recent call last):
  File "censored/.venv/bin/ansible-review", line 11, in <module>
    sys.exit(main())
  File "censored/.venv/local/lib/python2.7/site-packages/ansiblereview/main/__init__.py", line 95, in main
    errors = errors + candidate.review(options, lines)
  File "censored/.venv/local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 72, in review
    return utils.review(self, settings, lines)
  File "censored/.venv/local/lib/python2.7/site-packages/ansiblereview/utils/__init__.py", line 120, in review
    result = standard.check(candidate, settings)
  File "censored/.venv/local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 219, in ansiblelint
    matches = rules.run(fileinfo, rulename.split(','))
  File "censored/.venv/local/lib/python2.7/site-packages/ansiblelint/__init__.py", line 139, in run
    matches.extend(rule.matchtasks(playbookfile, text))
  File "censored/.venv/local/lib/python2.7/site-packages/ansiblelint/__init__.py", line 66, in matchtasks
    yaml = ansiblelint.utils.parse_yaml_linenumbers(text, file['path'])
  File "censored/.venv/local/lib/python2.7/site-packages/ansiblelint/utils.py", line 524, in parse_yaml_linenumbers
    data = loader.get_single_data()
  File "censored/.venv/local/lib/python2.7/site-packages/yaml/constructor.py", line 39, in get_single_data
    return self.construct_document(node)
  File "censored/.venv/local/lib/python2.7/site-packages/yaml/constructor.py", line 48, in construct_document
    for dummy in generator:
  File "censored/.venv/local/lib/python2.7/site-packages/yaml/constructor.py", line 398, in construct_yaml_map
    value = self.construct_mapping(node)
  File "censored/.venv/local/lib/python2.7/site-packages/ansiblelint/utils.py", line 515, in construct_mapping
    mapping = Constructor.construct_mapping(loader, node, deep=deep)
  File "censored/.venv/local/lib/python2.7/site-packages/yaml/constructor.py", line 208, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "censored/.venv/local/lib/python2.7/site-packages/yaml/constructor.py", line 132, in construct_mapping
    "found unacceptable key (%s)" % exc, key_node.start_mark)
yaml.constructor.ConstructorError: while constructing a mapping
  in "<unicode string>", line 54, column 13:
          port: {{ i_have_missing_double_quotes }}
                ^
found unacceptable key (unhashable type: 'dict')
  in "<unicode string>", line 54, column 14:
          port: {{ i_have_missing_double_quotes }}
                 ^

Expected behaviour:
no traceback but meaningful error explanation.

How do you get ansible-review to exit non-zero?

If a review fails. It exits with a return code 0. We want it to return non-zero to stop Jenkins from continuing with the build. Is it possible?

WARN: Couldn't classify file tox.ini

Regarding "WARN: Couldn't classify file", quick look at
candidate = classify(filename) and classify function make's me understand that if a file is not classified in the classify it's determined as None , as result ansible-review will always return a WARN for such files.

Should it have a mechanism to provide FILE we can ignore. Path ignore mechanism already exists for ansible-lint, i think ansible-review should use similar approach or use the existing config.

Inconsistent output from ansible-review for some rules

The output from ansible-review shows some inconsistent formatting. For example, some internal WARN messages are shown without a rule number reference. I believe all WARN and ERROR messages should produce the same formatted output and contain:

• tag (WARN or ERROR)
• description
• unique rule number (for reference purposes)
• line number
• specific info (e.g... variable X is declared twice)

Most of the lint rules follow this concept. Here are some internal rules that do not.

From init.py. Two warnings are raised in this code but again they should follow the same output requirements as described above:

    if not candidate.version:
        candidate.version = standards_latest(standards.standards)
        if candidate.expected_version:
            if isinstance(candidate, ansiblereview.RoleFile):
                warn("%s %s is in a role that contains a meta/main.yml without a declared "
                     "standards version. "
                     "Using latest standards version %s" %
                     (type(candidate).__name__, candidate.path, candidate.version),
                     settings)
            else:
                warn("%s %s does not present standards version. "
                     "Using latest standards version %s" %
                     (type(candidate).__name__, candidate.path, candidate.version),
                     settings)

They produce messages like this:

WARN: RoleVars vars/main.yml is in a role that contains a meta/main.yml without a declared standards version. Using latest standards version 0.1
WARN: Task tasks/main.yml is in a role that contains a meta/main.yml without a declared standards version. Using latest standards version 0.1

Also, another example is the following rule that checks YAML indentation. Here is the rule output:

WARN: Best practice "YAML should be correctly indented" not met:
vars/main.yml:4: indentation should increase by 2 chars

However, there is no rule number in the output.

Recommended changes:

• modify the internal warn() and error() functions so that you can provide all of this info when these are raised internally.
• modify the lint rules to include the right info