fqcn[deep]: detect deep plugins (#3502)

Author: ssbarnea

.github/workflows/tox.yml

@@ -59,7 +59,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: 802
+      PYTEST_REQPASS: 804
     steps:
       - name: Activate WSL1
         if: "contains(matrix.shell, 'wsl')"

examples/collection/plugins/modules/alpha.py

@@ -0,0 +1,44 @@
+"""An ansible test module."""
+
+
+DOCUMENTATION = """
+module: mod_1
+author:
+- test
+short_description: This is a test module
+description:
+- This is a test module
+version_added: 1.0.0
+options:
+  foo:
+    description:
+    - Dummy option I(foo)
+    type: str
+  bar:
+    description:
+    - Dummy option I(bar)
+    default: candidate
+    type: str
+    choices:
+    - candidate
+    - running
+    aliases:
+    - bam
+notes:
+- This is a dummy module
+"""
+
+EXAMPLES = """
+- name: test task-1
+  company_name.coll_1.mod_1:
+    foo: some value
+    bar: candidate
+"""
+
+RETURN = """
+baz:
+    description: test return 1
+    returned: success
+    type: list
+    sample: ['a','b']
+"""

examples/collection/plugins/modules/deep/beta.py

@@ -0,0 +1,44 @@
+"""An ansible test module."""
+
+
+DOCUMENTATION = """
+module: mod_2
+author:
+- test
+short_description: This is a test module
+description:
+- This is a test module
+version_added: 1.0.0
+options:
+  foo:
+    description:
+    - Dummy option I(foo)
+    type: str
+  bar:
+    description:
+    - Dummy option I(bar)
+    default: candidate
+    type: str
+    choices:
+    - candidate
+    - running
+    aliases:
+    - bam
+notes:
+- This is a dummy module
+"""
+
+EXAMPLES = """
+- name: test task-1
+  company_name.coll_1.mod_2:
+    foo: some value
+    bar: candidate
+"""
+
+RETURN = """
+baz:
+    description: test return 1
+    returned: success
+    type: list
+    sample: ['a','b']
+"""

src/ansiblelint/config.py

@@ -34,6 +34,7 @@
 DEFAULT_WARN_LIST = [
     "experimental",
     "jinja[spacing]",  # warning until we resolve all reported false-positives
+    "fqcn[deep]",  # 2023-05-31 added
 ]
 
 DEFAULT_KINDS = [
@@ -74,6 +75,11 @@
     {"yaml": "**/*.{yaml,yml}"},
     {"yaml": "**/.*.{yaml,yml}"},
     {"sanity-ignore-file": "**/tests/sanity/ignore-*.txt"},
+    # what are these doc_fragments? We also ignore module_utils for now
+    {
+        "plugin": "**/plugins/{action,become,cache,callback,connection,filter,inventory,lookup,modules,test}/**/*.py",
+    },
+    {"python": "**/*.py"},
 ]
 
 BASE_KINDS = [
@@ -93,6 +99,7 @@
     {"text/yaml": "**/{.ansible-lint,.yamllint}"},
     {"text/yaml": "**/*.{yaml,yml}"},
     {"text/yaml": "**/.*.{yaml,yml}"},
+    {"text/python": "**/*.py"},
 ]
 
 PROFILES = yaml_from_file(Path(__file__).parent / "data" / "profiles.yml")

src/ansiblelint/constants.py

@@ -72,6 +72,7 @@ def main():
     "yaml",  # generic yaml file, previously reported as unknown file type
     "ansible-lint-config",
     "sanity-ignore-file",  # tests/sanity/ignore file
+    "plugin",
     "",  # unknown file type
 ]
 

src/ansiblelint/file_utils.py

@@ -400,8 +400,9 @@ def data(self) -> Any:
                     self.state = append_skipped_rules(self.state, self)
                 else:
                     logging.debug(
-                        "data set to None for %s due to being of %s kind.",
+                        "data set to None for %s due to being '%s' (%s) kind.",
                         self.path,
+                        self.kind,
                         self.base_kind or "unknown",
                     )
                     self.state = States.UNKNOWN_DATA

src/ansiblelint/rules/fqcn.md

@@ -12,6 +12,8 @@ The `fqcn` rule has the following checks:
 - `fqcn[action-core]` - Checks for FQCNs from the `ansible.legacy` or
   `ansible.builtin` collection.
 - `fqcn[canonical]` - You should use canonical module name ... instead of ...
+- [`fqcn[deep]`](#deep-modules) - Checks for deep/nested plugins directory
+  inside collections.
 - `fqcn[keyword]` - Avoid `collections` keyword by using FQCN for all plugins,
   modules, roles and playbooks.
 
@@ -41,6 +43,17 @@ compatible with a very old version of Ansible, one that does not know how to
 resolve that name. If you find yourself in such a situation, feel free to add
 this rule to the ignored list.
 
+## Deep modules
+
+When writing modules, you should avoid nesting them in deep directories, even if
+Ansible allows you to do so. Since early 2023, the official guidance, backed by
+the core team, is to use a flat directory structure for modules. This ensures
+optimal performance.
+
+Existing collections that still use deep directories can migrate to the flat
+structure in a backward-compatible way by adding redirects like in
+[this example](https://github.com/ansible-collections/community.general/blob/main/meta/runtime.yml#L227-L233).
+
 ## Problematic Code
 
 ```yaml

src/ansiblelint/rules/fqcn.py

@@ -173,6 +173,29 @@ def matchtask(
                     )
         return result
 
+    def matchyaml(self, file: Lintable) -> list[MatchError]:
+        """Return matches found for a specific YAML text."""
+        result = []
+        if file.kind == "plugin":
+            i = file.path.resolve().parts.index("plugins")
+            plugin_type = file.path.resolve().parts[i : i + 2]
+            short_path = file.path.resolve().parts[i + 2 :]
+            if len(short_path) > 1:
+                result.append(
+                    self.create_matcherror(
+                        message=f"Deep plugins directory is discouraged. Move '{file.path}' directly under '{'/'.join(plugin_type)}' folder.",
+                        tag="fqcn[deep]",
+                        filename=file,
+                    ),
+                )
+        elif file.kind == "playbook":
+            for play in file.data:
+                if play is None:
+                    continue
+
+                result.extend(self.matchplay(file, play))
+        return result
+
     def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
         if file.kind != "playbook":
             return []
@@ -241,3 +264,21 @@ def test_fqcn_builtin_pass() -> None:
         success = "examples/playbooks/rule-fqcn-pass.yml"
         results = Runner(success, rules=collection).run()
         assert len(results) == 0, results
+
+    def test_fqcn_deep_fail() -> None:
+        """Test rule matches."""
+        collection = RulesCollection()
+        collection.register(FQCNBuiltinsRule())
+        failure = "examples/collection/plugins/modules/deep/beta.py"
+        results = Runner(failure, rules=collection).run()
+        assert len(results) == 1
+        assert results[0].tag == "fqcn[deep]"
+        assert "Deep plugins directory is discouraged" in results[0].message
+
+    def test_fqcn_deep_pass() -> None:
+        """Test rule does not match."""
+        collection = RulesCollection()
+        collection.register(FQCNBuiltinsRule())
+        success = "examples/collection/plugins/modules/alpha.py"
+        results = Runner(success, rules=collection).run()
+        assert len(results) == 0