Improve cli argument handling (#2099)

* Include opt-in rules when listing tags and rules

With PR #1450 optional rules with the 'opt-in' tag were introduced
and according to the docs, listing rules and tags should also list
the opt-in rules.

Fixes: #2068

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>

* gracefully handle invalid format options with listing rules

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>

* put arguments -L and -T into a mutually exclusive group

We either print a list of rules or a list of tags, but never
both at the same time. So it makes no sense to allow giving
both arguments at the same time.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>

* always provide a long form of cli arguments

Having short cli arguments for often used options is nice to have
when using the command in an interactive shell. For writing scripts
it would be better to have long forms of arguments as it increases
the readebility.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>

* Update tox.yml

Co-authored-by: Sorin Sbarnea <sorin.sbarnea@gmail.com>

Author: ziegenberg

.github/workflows/tox.yml

@@ -155,7 +155,8 @@ jobs:
       WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:TOX_PARALLEL_NO_SPINNER
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 611
+      PYTEST_REQPASS: 621
+
     steps:
       - name: Activate WSL1
         if: "contains(matrix.shell, 'wsl')"

docs/rules.rst

@@ -27,7 +27,7 @@ The following shows the available tags in an example set of rules, and the
 rules associated with each tag:
 
 
-.. command-output:: ansible-lint -v -T
+.. command-output:: ansible-lint -T
    :cwd: ..
    :returncode: 0
    :nostderr:

src/ansiblelint/cli.py

@@ -213,15 +213,27 @@ def get_cli_parser() -> argparse.ArgumentParser:
     """Initialize an argument parser."""
     parser = argparse.ArgumentParser()
 
-    parser.add_argument(
+    listing_group = parser.add_mutually_exclusive_group()
+    listing_group.add_argument(
         "-L",
+        "--list-rules",
         dest="listrules",
         default=False,
         action="store_true",
-        help="list all the rules",
+        help="List all the rules. For listing rules only the following formats "
+        "for argument -f are supported: {plain, rich, rst}",
+    )
+    listing_group.add_argument(
+        "-T",
+        "--list-tags",
+        dest="listtags",
+        action="store_true",
+        help="List all the tags and the rules they cover. Increase the verbosity level "
+        "with `-v` to include 'opt-in' tag and its rules.",
     )
     parser.add_argument(
         "-f",
+        "--format",
         dest="format",
         default="rich",
         choices=[
@@ -245,6 +257,7 @@ def get_cli_parser() -> argparse.ArgumentParser:
     )
     parser.add_argument(
         "-p",
+        "--parseable",
         dest="parseable",
         default=False,
         action="store_true",
@@ -268,6 +281,7 @@ def get_cli_parser() -> argparse.ArgumentParser:
     )
     parser.add_argument(
         "-r",
+        "--rules-dir",
         action=AbspathArgAction,
         dest="rulesdir",
         default=[],
@@ -310,14 +324,12 @@ def get_cli_parser() -> argparse.ArgumentParser:
     )
     parser.add_argument(
         "-t",
+        "--tags",
         dest="tags",
         action="append",
         default=[],
         help="only check rules whose id/tags match these values",
     )
-    parser.add_argument(
-        "-T", dest="listtags", action="store_true", help="list all the tags"
-    )
     parser.add_argument(
         "-v",
         dest="verbosity",
@@ -327,13 +339,15 @@ def get_cli_parser() -> argparse.ArgumentParser:
     )
     parser.add_argument(
         "-x",
+        "--skip-list",
         dest="skip_list",
         default=[],
         action="append",
         help="only check rules whose id/tags do not " "match these values",
     )
     parser.add_argument(
         "-w",
+        "--warn-list",
         dest="warn_list",
         default=[],
         action="append",
@@ -372,6 +386,7 @@ def get_cli_parser() -> argparse.ArgumentParser:
     )
     parser.add_argument(
         "-c",
+        "--config-file",
         dest="config_file",
         help="Specify configuration file to use. By default it will look for '.ansible-lint' or '.config/ansible-lint.yml'",
     )
@@ -482,6 +497,13 @@ def get_config(arguments: List[str]) -> Namespace:
     parser = get_cli_parser()
     options = parser.parse_args(arguments)
 
+    if options.listrules and options.format not in ["plain", "rich", "rst"]:
+        parser.error(
+            f"argument -f: invalid choice: '{options.format}'. "
+            f"In combination with argument -L only 'plain', "
+            f"'rich' or 'rst' are supported with -f."
+        )
+
     file_config = load_config(options.config_file)
 
     config = merge_config(file_config, options)

src/ansiblelint/rules/__init__.py

@@ -344,8 +344,16 @@ def __init__(
 
     def register(self, obj: AnsibleLintRule) -> None:
         """Register a rule."""
-        # We skip opt-in rules which were not manually enabled
-        if "opt-in" not in obj.tags or obj.id in self.options.enable_list:
+        # We skip opt-in rules which were not manually enabled.
+        # But we do include opt-in rules when listing all rules or tags
+        if any(
+            [
+                "opt-in" not in obj.tags,
+                obj.id in self.options.enable_list,
+                self.options.listrules,
+                self.options.listtags,
+            ]
+        ):
             self.rules.append(obj)
 
     def __iter__(self) -> Iterator[BaseRule]:

test/fixtures/list-rules-tests/.yamllint

@@ -0,0 +1,2 @@
+---
+{}

test/test_list_rules.py

@@ -0,0 +1,77 @@
+"""Tests related to our logging/verbosity setup."""
+
+import os
+
+import pytest
+
+from ansiblelint.testing import run_ansible_lint
+
+
+def test_list_rules_includes_opt_in_rules() -> None:
+    """Checks that listing rules also includes the opt-in rules."""
+    # Piggyback off the .yamllint in the root of the repo, just for testing.
+    # We'll "override" it with the one in the fixture.
+    cwd = os.path.realpath(
+        os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")
+    )
+    fakerole = os.path.join("test", "fixtures", "list-rules-tests")
+
+    result_list_rules = run_ansible_lint("-L", fakerole, cwd=cwd)
+
+    assert ("opt-in" in result_list_rules.stdout) is True
+
+
+@pytest.mark.parametrize(
+    ("result", "returncode", "format_string"),
+    (
+        (False, 0, "plain"),
+        (False, 0, "rich"),
+        (False, 0, "rst"),
+        (True, 2, "json"),
+        (True, 2, "codeclimate"),
+        (True, 2, "quiet"),
+        (True, 2, "pep8"),
+        (True, 2, "foo"),
+    ),
+    ids=(
+        "plain",
+        "rich",
+        "rst",
+        "json",
+        "codeclimate",
+        "quiet",
+        "pep8",
+        "foo",
+    ),
+)
+def test_list_rules_with_format_option(
+    result: bool, returncode: int, format_string: str
+) -> None:
+    """Checks that listing rules with format options works."""
+    # Piggyback off the .yamllint in the root of the repo, just for testing.
+    # We'll "override" it with the one in the fixture.
+    cwd = os.path.realpath(
+        os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")
+    )
+    fakerole = os.path.join("test", "fixtures", "list-rules-tests")
+
+    result_list_rules = run_ansible_lint("-f", format_string, "-L", fakerole, cwd=cwd)
+    print(result_list_rules.stdout)
+
+    assert (f"invalid choice: '{format_string}'" in result_list_rules.stderr) is result
+    assert ("syntax-check" in result_list_rules.stdout) is not result
+    assert result_list_rules.returncode is returncode
+
+
+def test_list_tags_includes_opt_in_rules() -> None:
+    """Checks that listing tags also includes the opt-in rules."""
+    # Piggyback off the .yamllint in the root of the repo, just for testing.
+    # We'll "override" it with the one in the fixture.
+    cwd = os.path.realpath(
+        os.path.join(os.path.dirname(os.path.realpath(__file__)), "..")
+    )
+    fakerole = os.path.join("test", "fixtures", "list-rules-tests")
+
+    result_list_tags = run_ansible_lint("-L", fakerole, cwd=cwd)
+
+    assert ("opt-in" in result_list_tags.stdout) is True