add rule to flag short-hand syntax

[reduzent]

Issue Type

• Feature request

Ansible and Ansible Lint details

ansible 2.8.5
ansible-lint 4.1.0

• ansible installation method: pip
• ansible-lint installation method: pip

Desired Behaviour

Add a rule to flag lines that make use of the old ansible short-hand syntax.

This would be flagged:

- name: configure service
  template: src=config.file.j2 dest=/etc/service/config.cfg

while this won't:

- name: configure service
  template:
    src: config.file.j2
    dest: /etc/service/config.cfg

[ssbarnea]

That one is now quite easy to implement and very useful as we should really discourage use of the old short syntax, as it is almost impossible to get it integrated with editors and YAML/JSON schema verification.

I am personally even considering a more extreme move in which we would even remove the parsing of short-hand syntax from the linter and only keep that new rule. The only downside is that it may upset few people that used debug module with shorthand.

[cognifloyd]

I'm planning to implement this. Has anyone else already worked on it?

[ssbarnea]

I'm planning to implement this. Has anyone else already worked on it?

Go for it! I will be happy to review that useful feature.

[reduzent]

We use the following custom linter rule:

from ansiblelint import AnsibleLintRule
import re

class ShorthandSyntaxRule(AnsibleLintRule):
    id = '299'
    shortdesc = 'Shorthand syntax is discouraged'
    description = 'Instead of old-school shorthand syntax, use canonical YAML syntax'
    severity = 'LOW'
    tags = ['formatting', 'readability']
    version_added = 'extra'

    def match(self, file, line):
        allowed_keys = ['command', 'line', 'mode', 'opts', 'regexp', 'validate', 'when']
        rule = r'^\s*-?\s*(?!(' + '|'.join(allowed_keys) + r'))\S+: \S+=\S'
        p = re.compile(rule)
        return p.match(line)

However, I'm not convinced it should go into the official ruleset. It detects too many false positives. Maybe someone can improve it?

[tadeboro]

Making this a task-based check would be more accurate and more manageable. I am a bit rusty when it comes to lint' internals, but I think replacing the match with the following rudimentary matchtask function would do the job:

def matchtask(self, task, file=None):
    return (
        task['__ansible_action_type__'] in ('task', 'handler') and
        isinstance(task['action']['__ansible_arguments__'], list)
    )

But as I said before, it has been a while since I worked on linter, so make sure to validate my code thoroughly.

[cognifloyd]

Yeah. It turns out the original task line is not available in matchtask()'s task because it gets normalized via ansible into actual arguments (top level on task). But I have something working that re-parses the yaml file. I think I'll step back and override matchtasks() instead (copying most of the implementation from matchtasks().

[cognifloyd]

Here is my rule: https://github.com/cognifloyd/ansible-lint/blob/fmt/src/ansiblelint/rules/TaskNoActionShorthand.py

class TaskNoActionShorthand(AnsibleLintRule):

    id = "no-action-shorthand"
    shortdesc = "Use YAML args instead of action shorthand."
    description = (
        "Use YAML args instead of action shorthand.\n"
        "Instead of ``module: arg1=value arg2=42``, use:\n"
        "  module:\n"
        "    arg1: value\n"
        "    arg2: 42\n"
        "Early versions of Ansible used a shorthand to define args, but "
        "(1) action shorthand relies on Ansible's magic type casting "
        "which is the source of many obscure, difficult-to-debug issues; and "
        "(2) schema based linting cannot detect issues when args are hidden "
        "in the action shorthand. "
    )
    # Action shorthand was removed from ansible's documentation after v2.9
    # https://docs.ansible.com/ansible/2.9/user_guide/playbooks_intro.html#action-shorthand
    severity = 'MEDIUM'
    tags = ['idiom']
    version_added = "5.3"

    needs_raw_task = True

    def matchrawtask(
        self,
        raw_task: Dict[str, Any],
        task: Dict[str, Any],
        file: Optional[Lintable] = None,
    ) -> Union[bool, str]:
        if task["action"]["__ansible_module__"] in FREE_FORM_MODULES:
            return False

        module = task["action"]["__ansible_module_original__"]
        raw_action_block = raw_task[module]
        if isinstance(raw_action_block, str):
            return True
        # raw_action_block should be a dict, which is what we want.
        # if isinstance(raw_action_block, dict):
        #     return False

        return False

To make that work, I modified BaseRule and AnsibleLintRule: cognifloyd@a9a6efe

Aside: I'm building a fmt mode for ansible-lint. This is part of that effort. Now to write the transform that updates the playbooks to replace the shorthand format with the YAML style args. All of that work is in this branch: https://github.com/cognifloyd/ansible-lint/tree/fmt (main...cognifloyd:fmt)

[cognifloyd]

As a prerequisite to getting this rule in, I filed two PRs, alternative ways to pass the raw task into the rule. #1833 and #1834 (only one of these should be merged)

[gtirloni]

Great work! Are you planning to submit the actual rule too soon? Thank you.

[ssbarnea]

We don't really need a new rule to implement this one, we can implement it by updating the schemas.

There is a pull request to add this directly to our schemas at ansible/schemas#310 and that will enforce it not only on the linter but also on vscode-ansible extension.

[ssbarnea]

That is true, the only way to avoid it would be to disable the entire "schema" rule.

We will review the effects of this change well before deciding on it, especially as it is disrupting. The tricky bit is that we need to reduce the complexity of linter analysis by supporting only normalized syntax. There is a huge amount of code in linter to cope with historical variations that piled up.

IMHO, shorthand syntax should have being limited to command line use and never implemented for files.

How many people were hit by surprise behaviors with the stuff below:

- shell: echo creates=foo.txt

One equal in you shorthand syntax and what goes in is no longer the text you wrote.

I am considering a trade-off approach where we might still ignore shorthand string syntax if it does not have any equal in it. I am not yet sure if it would work, but if it does we might be able to force people to drop shorthand only when they want to add arguments. At least that would be less intrusive. I don't like adding cmd: key to tons of shell/command modules either and I doubt others are, especially when they do not really use foo=bar argument passing.

[cidrblock]

I am in favor of not allowing short-hard style task parameters, I needed to do this recently and will say I think it really improved the structure and readability of the task files: https://github.com/ansible-collections/cisco.nxos/pull/497/files.

for ref: #2117

Rule vs schema.... this is tricky, the schema keeps the code base more manageable and give immediate feedback in the IDE (vscode) but the all-or-nothing of the schema might put people off

[cidrblock]

Just to note, in case anyone was curious, that cleanup wasn't done by hand :) This was used: https://github.com/cidrblock/collection_test_cleaner/blob/main/clean.py#L426 which I think we might be able to add to lint at some point to do selective cleanup and rewriting

[felixfontein]

I'm totally in favour of not using the shorthand form, and of having a way to flag places where it is still used. But treating them as syntax errors (without a way to disable that specific problem in some places, or generally) is way too extreme IMO.

(And having some automation to convert the shorthand form to the long form would be even better!)

[felixfontein]

How many people were hit by surprise behaviors with the stuff below:

- shell: echo creates=foo.txt

One equal in you shorthand syntax and what goes in is no longer the text you wrote.

Since this only happens for very special words before =, I guess not that many. For example shell: echo a=b works flawlessly.

[MarkusTeufelberger]

Implemented in #2527