diff --git a/.config/dictionary.txt b/.config/dictionary.txt index 8f5680403d..157c54315f 100644 --- a/.config/dictionary.txt +++ b/.config/dictionary.txt @@ -135,7 +135,6 @@ firewalld fontawesome formatstr formetting -formsyntax fqcn fqrn fulltoc diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index d55812cee8..3567d1cbd3 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -70,7 +70,7 @@ jobs: WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 809 + PYTEST_REQPASS: 808 steps: - name: Activate WSL1 if: "contains(matrix.shell, 'wsl')" diff --git a/docs/profiles.md b/docs/profiles.md index 95e4598f4b..a3c309a162 100644 --- a/docs/profiles.md +++ b/docs/profiles.md @@ -31,13 +31,13 @@ and formatting. It extends [min](#min) profile. - [command-instead-of-module](rules/command-instead-of-module.md) - [command-instead-of-shell](rules/command-instead-of-shell.md) - [deprecated-bare-vars](rules/deprecated-bare-vars.md) -- [deprecated-command-syntax](rules/deprecated-command-syntax.md) - [deprecated-local-action](rules/deprecated-local-action.md) - [deprecated-module](rules/deprecated-module.md) - [inline-env-var](rules/inline-env-var.md) - [key-order](rules/key-order.md) - [literal-compare](rules/literal-compare.md) - [jinja](rules/jinja.md) +- [no-free-form](https://github.com/ansible/ansible-lint/issues/2117) - [no-jinja-when](rules/no-jinja-when.md) - [no-tabs](rules/no-tabs.md) - [partial-become](rules/partial-become.md) @@ -56,7 +56,6 @@ content easier to read and maintain. It extends [basic](#basic) profile. - [name[template]](rules/name.md) - [name[imperative]](https://github.com/ansible/ansible-lint/issues/2170) - [name[casing]](rules/name.md) -- [no-free-form](https://github.com/ansible/ansible-lint/issues/2117) - [spell-var-name](https://github.com/ansible/ansible-lint/issues/2168) ## safety diff --git a/docs/redirects.yml b/docs/redirects.yml index cda53eec01..744869c37d 100644 --- a/docs/redirects.yml +++ b/docs/redirects.yml @@ -7,6 +7,9 @@ - type: page from_url: /rules/hg-latest/ to_url: /rules/latest/ +- type: page + from_url: /rules/deprecated-command-syntax/ + to_url: /rules/no-free-form/ - type: page from_url: /default_rules/index/ to_url: /rules/ diff --git a/docs/rules/deprecated-command-syntax.md b/docs/rules/deprecated-command-syntax.md deleted file mode 120000 index 70cb557401..0000000000 --- a/docs/rules/deprecated-command-syntax.md +++ /dev/null @@ -1 +0,0 @@ -../../src/ansiblelint/rules/deprecated_command_syntax.md \ No newline at end of file diff --git a/docs/rules/index.md b/docs/rules/index.md index dd4d057c3d..2315f86da6 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -5,7 +5,6 @@ - [command-instead-of-module][] - [command-instead-of-shell][] - [deprecated-bare-vars][] -- [deprecated-command-syntax][] - [deprecated-local-action][] - [deprecated-module][] - [empty-string-compare][] diff --git a/docs/usage.md b/docs/usage.md index 6c749b1ac7..033629db4d 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -230,7 +230,7 @@ a task, which does not skip line-based rules. You can use the _command_ or _shell_ modules, for example: ```yaml -- name: This would typically fire deprecated-command-syntax +- name: This would typically fire no-free-form command: warn=no chmod 644 X - name: This would typically fire command-instead-of-module diff --git a/examples/playbooks/rule-deprecated-command-syntax.yml b/examples/playbooks/rule-deprecated-command-syntax.yml new file mode 100644 index 0000000000..a7b9807a83 --- /dev/null +++ b/examples/playbooks/rule-deprecated-command-syntax.yml @@ -0,0 +1,7 @@ +--- +- name: Fixture + hosts: localhost + tasks: + - name: Shell with pipe + ansible.builtin.command: creates=/tmp/foo touch /tmp/foo + changed_when: false diff --git a/examples/playbooks/skiptasks.yml b/examples/playbooks/skiptasks.yml index 7a3b09db56..e105ed3081 100644 --- a/examples/playbooks/skiptasks.yml +++ b/examples/playbooks/skiptasks.yml @@ -12,7 +12,7 @@ ansible.builtin.command: git log changed_when: false - - name: Test deprecated-command-syntax + - name: Test no-free-form ansible.builtin.command: creates=B chmod 644 A - name: Test latest[git] (skip) @@ -30,7 +30,7 @@ tags: - skip_ansible_lint - - name: Test deprecated-command-syntax (skip) + - name: Test no-free-form (skip) ansible.builtin.command: chmod 644 A tags: - skip_ansible_lint diff --git a/examples/playbooks/test_skip_inside_yaml.yml b/examples/playbooks/test_skip_inside_yaml.yml index 44d6c28dc2..1f7295442a 100644 --- a/examples/playbooks/test_skip_inside_yaml.yml +++ b/examples/playbooks/test_skip_inside_yaml.yml @@ -4,16 +4,30 @@ tags: - skip_ansible_lint # should disable error at playbook level tasks: - - name: Test # <-- 0 latest[hg] - action: ansible.builtin.hg # noqa: fqcn[canonical] - - name: Test latest[hg] (skipped) # noqa: latest[hg] fqcn[canonical] - action: ansible.builtin.hg + - name: Test + action: community.general.hg + args: + repo: foo + version: HEAD - - name: Test latest[git] and partial-become + - name: Test latest[hg] (skipped) + action: community.general.hg + args: + repo: foo + # revision: HEAD + + - name: Test latest[git] and partial-become # noqa: latest[git] action: ansible.builtin.git + args: + repo: foo + version: HEAD become_user: alice + - name: Test latest[git] and partial-become (skipped) # noqa: latest[git] partial-become action: ansible.builtin.git + args: + repo: foo + version: HEAD become_user: alice - name: Test YAML # <-- 1 jinja[spacing] @@ -27,13 +41,13 @@ url: http://example.com/really_long_path/really_long_path/really_long_path/really_long_path/really_long_path/really_long_path/really_long_path/really_long_path/file.conf # noqa: yaml[line-length] dest: "{{dest_proj_path}}/foo.conf" # noqa: jinja[spacing] - - name: Test deprecated-command-syntax # <-- 3 deprecated-command-syntax + - name: Test no-free-form # <-- 3 no-free-form ansible.builtin.command: creates=B chmod 644 A # noqa: no-free-form - - name: Test deprecated-command-syntax # <-- 4 deprecated-command-syntax + - name: Test no-free-form # <-- 4 no-free-form ansible.builtin.command: warn=yes creates=B chmod 644 A # noqa: no-free-form - - name: Test deprecated-command-syntax (skipped via no warn) + - name: Test no-free-form (skipped via no warn) ansible.builtin.command: warn=no creates=B chmod 644 A # noqa: no-free-form - - name: Test deprecated-command-syntax (skipped via skip_ansible_lint) + - name: Test no-free-form (skipped via skip_ansible_lint) ansible.builtin.command: creates=B chmod 644 A # noqa: no-free-form tags: - skip_ansible_lint diff --git a/mkdocs.yml b/mkdocs.yml index 817e8793b1..547d67649b 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -77,7 +77,6 @@ nav: - rules/command-instead-of-module.md - rules/command-instead-of-shell.md - rules/deprecated-bare-vars.md - - rules/deprecated-command-syntax.md - rules/deprecated-local-action.md - rules/deprecated-module.md - rules/empty-string-compare.md diff --git a/src/ansiblelint/constants.py b/src/ansiblelint/constants.py index 3b4a0ebc42..c618bff238 100644 --- a/src/ansiblelint/constants.py +++ b/src/ansiblelint/constants.py @@ -102,13 +102,14 @@ def main(): "703": "meta-incorrect", "704": "meta-video-links", "911": "syntax-check", - "var-spacing": "jinja[spacing]", - "unnamed-task": "name[missing]", + "deprecated-command-syntax": "no-free-form", + "fqcn-builtins": "fqcn[action-core]", "git-latest": "latest[git]", "hg-latest": "latest[hg]", "no-jinja-nesting": "jinja[invalid]", "no-loop-var-prefix": "loop-var-prefix", - "fqcn-builtins": "fqcn[action-core]", + "unnamed-task": "name[missing]", + "var-spacing": "jinja[spacing]", } PLAYBOOK_TASK_KEYWORDS = [ diff --git a/src/ansiblelint/data/profiles.yml b/src/ansiblelint/data/profiles.yml index 103795e797..225b3c825c 100644 --- a/src/ansiblelint/data/profiles.yml +++ b/src/ansiblelint/data/profiles.yml @@ -21,13 +21,14 @@ basic: command-instead-of-module: command-instead-of-shell: deprecated-bare-vars: - deprecated-command-syntax: deprecated-local-action: deprecated-module: inline-env-var: key-order: literal-compare: jinja: + no-free-form: # schema-related + url: https://github.com/ansible/ansible-lint/issues/2117 no-jinja-when: no-tabs: partial-become: @@ -49,8 +50,6 @@ moderate: name[imperative]: url: https://github.com/ansible/ansible-lint/issues/2170 name[casing]: - no-free-form: # schema-related - url: https://github.com/ansible/ansible-lint/issues/2117 spell-var-name: url: https://github.com/ansible/ansible-lint/issues/2168 safety: diff --git a/src/ansiblelint/rules/deprecated_command_syntax.md b/src/ansiblelint/rules/deprecated_command_syntax.md deleted file mode 100644 index f99ca9951b..0000000000 --- a/src/ansiblelint/rules/deprecated_command_syntax.md +++ /dev/null @@ -1,32 +0,0 @@ -# deprecated-command-syntax - -This rule identifies the use of shorthand (free-form) syntax as this is highly -discouraged inside playbooks, mainly because it can easily lead to bugs that -are hard to identify. - -While using the free-form from the command line is ok, it should never be used -inside playbooks. - -## Problematic Code - -```yaml ---- -- name: Example playbook - hosts: localhost - tasks: - - name: Perform chmod - ansible.builtin.command: creates=B chmod 644 A # <-- do not use shorthand -``` - -## Correct Code - -```yaml ---- -- name: Example playbook - hosts: localhost - tasks: - - name: Perform chmod - ansible.builtin.command: chmod 644 A - args: - creates: B -``` diff --git a/src/ansiblelint/rules/deprecated_command_syntax.py b/src/ansiblelint/rules/deprecated_command_syntax.py deleted file mode 100644 index 53ed94f10b..0000000000 --- a/src/ansiblelint/rules/deprecated_command_syntax.py +++ /dev/null @@ -1,101 +0,0 @@ -"""Implementation of deprecated-command-syntax rule.""" -# Copyright (c) 2013-2014 Will Thames -# -# Permission is hereby granted, free of charge, to any person obtaining a copy -# of this software and associated documentation files (the "Software"), to deal -# in the Software without restriction, including without limitation the rights -# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -# copies of the Software, and to permit persons to whom the Software is -# furnished to do so, subject to the following conditions: -# -# The above copyright notice and this permission notice shall be included in -# all copies or substantial portions of the Software. -# -# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -# THE SOFTWARE. - -from __future__ import annotations - -import os -import sys -from typing import TYPE_CHECKING, Any - -from ansiblelint.rules import AnsibleLintRule -from ansiblelint.utils import convert_to_boolean, get_first_cmd_arg - -if TYPE_CHECKING: - from ansiblelint.file_utils import Lintable - - -class CommandsInsteadOfArgumentsRule(AnsibleLintRule): - """Using command rather than an argument to e.g. file.""" - - id = "deprecated-command-syntax" - description = ( - "Executing a command when there are arguments to modules " - "is generally a bad idea" - ) - severity = "VERY_HIGH" - tags = ["command-shell", "deprecations"] - version_added = "historic" - - _commands = ["command", "shell", "raw"] - _arguments = { - "chown": "owner", - "chmod": "mode", - "chgrp": "group", - "ln": "state=link", - "mkdir": "state=directory", - "rmdir": "state=absent", - "rm": "state=absent", - } - - def matchtask( - self, task: dict[str, Any], file: Lintable | None = None - ) -> bool | str: - if task["action"]["__ansible_module__"] in self._commands: - first_cmd_arg = get_first_cmd_arg(task) - if not first_cmd_arg: - return False - - executable = os.path.basename(first_cmd_arg) - if executable in self._arguments and convert_to_boolean( - task["action"].get("warn", True) - ): - message = "{0} used in place of argument {1} to file module" - return message.format(executable, self._arguments[executable]) - return False - - -DEPRECATED_COMMAND_PLAY = """--- -- name: Fixture - hosts: localhost - tasks: - - name: Shell with pipe - ansible.builtin.command: - err: echo hello | true # noqa: risky-shell-pipe - changed_when: false -""" - - -# testing code to be loaded only with pytest or when executed the rule file -if "pytest" in sys.modules: - import pytest - - from ansiblelint.testing import RunFromText # pylint: disable=ungrouped-imports - - @pytest.mark.parametrize( - ("text", "expected"), - (pytest.param(DEPRECATED_COMMAND_PLAY, 0, id="no_first_cmd_arg"),), - ) - def test_rule_deprecated_command_no_first_cmd_arg( - default_text_runner: RunFromText, text: str, expected: int - ) -> None: - """Validate that rule works as intended.""" - results = default_text_runner.run_playbook(text) - assert len(results) == expected diff --git a/src/ansiblelint/rules/no_free_form.md b/src/ansiblelint/rules/no_free_form.md index 3ce514034f..0ffc0acbda 100644 --- a/src/ansiblelint/rules/no_free_form.md +++ b/src/ansiblelint/rules/no_free_form.md @@ -11,19 +11,19 @@ autocomplete and validation for the edited line. !!! note As long you just pass a YAML string that contains a `=` character inside as the - parameter to the action module name, we consider this as using free-formsyntax. + parameter to the action module name, we consider this as using free-form syntax. Be sure you pass a dictionary to the module, so the free-form parsing is never triggered. As `raw` module only accepts free-form, we trigger `no-free-form[raw]` only if -we detect the presence of `executable=` inside raw calls. We advice the explicit -use of `args:` dictionary for configuring the executable to be run. +we detect the presence of `executable=` inside raw calls. We advise the explicit +use of `args:` for configuring the executable to be run. -This rule can produce messages such: +This rule can produce messages as: - `no-free-form` - Free-form syntax is discouraged. -- `no-free-form[raw-non-string]` - Passing a non string value to `raw` module is - neither documented or supported. +- `no-free-form[raw-non-string]` - Passing a non-string value to `raw` module is + neither documented nor supported. ## Problematic code diff --git a/src/ansiblelint/schemas/ansible-lint-config.json b/src/ansiblelint/schemas/ansible-lint-config.json index 0ad202e7e4..10917761bc 100644 --- a/src/ansiblelint/schemas/ansible-lint-config.json +++ b/src/ansiblelint/schemas/ansible-lint-config.json @@ -128,7 +128,6 @@ "command-instead-of-module", "command-instead-of-shell", "deprecated-bare-vars", - "deprecated-command-syntax", "deprecated-local-action", "deprecated-module", "empty-string-compare", diff --git a/test/schemas/negative_test/.ansible-lint.md b/test/schemas/negative_test/.ansible-lint.md index 22538e0e4a..7801d6dbeb 100644 --- a/test/schemas/negative_test/.ansible-lint.md +++ b/test/schemas/negative_test/.ansible-lint.md @@ -11,7 +11,6 @@ "command-instead-of-module", "command-instead-of-shell", "deprecated-bare-vars", - "deprecated-command-syntax", "deprecated-local-action", "deprecated-module", "empty-string-compare", diff --git a/test/test_rules_collection.py b/test/test_rules_collection.py index 1939d253b0..60a2b83cc0 100644 --- a/test/test_rules_collection.py +++ b/test/test_rules_collection.py @@ -166,5 +166,5 @@ def test_rules_id_format() -> None: rule.help != "" or rule.description or rule.__doc__ ), f"Rule {rule.id} must have at least one of: .help, .description, .__doc__" assert "yaml" in keys, "yaml rule is missing" - assert len(rules) == 51 # update this number when adding new rules! + assert len(rules) == 50 # update this number when adding new rules! assert len(keys) == len(rules), "Duplicate rule ids?" diff --git a/test/test_skip_inside_yaml.py b/test/test_skip_inside_yaml.py index d90c45dc51..c35268d0dc 100644 --- a/test/test_skip_inside_yaml.py +++ b/test/test_skip_inside_yaml.py @@ -55,7 +55,7 @@ def test_role_tasks_with_block(default_text_runner: RunFromText) -> None: @pytest.mark.parametrize( ("lintable", "expected"), - (pytest.param("examples/playbooks/test_skip_inside_yaml.yml", 10, id="yaml"),), + (pytest.param("examples/playbooks/test_skip_inside_yaml.yml", 2, id="yaml"),), ) def test_inline_skips( default_rules_collection: RulesCollection, lintable: str, expected: int