From 379643b903020dbdf8fd817d683cab89a52927d7 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Thu, 13 Aug 2026 16:05:33 -0700 Subject: [PATCH 1/2] fix: polymath-ansible hook can run in repos with outside requirements Signed-off-by: Emerson Knapp --- polymath_code_standard/checkers/ansible.py | 118 +++++++++++++++++- .../config/ansible-lint.yml | 7 ++ pyproject.toml | 6 + test_files/ansible/inventory.yml | 5 + test_files/ansible/playbook.yml | 20 +++ test_files/ansible/requirements.yml | 7 ++ tests/test_runner.py | 50 ++++++++ 7 files changed, 210 insertions(+), 3 deletions(-) create mode 100644 test_files/ansible/inventory.yml create mode 100644 test_files/ansible/playbook.yml create mode 100644 test_files/ansible/requirements.yml diff --git a/polymath_code_standard/checkers/ansible.py b/polymath_code_standard/checkers/ansible.py index df926f7..2578094 100644 --- a/polymath_code_standard/checkers/ansible.py +++ b/polymath_code_standard/checkers/ansible.py @@ -1,21 +1,133 @@ # SPDX-FileCopyrightText: 2026 Polymath Robotics, Inc. # SPDX-License-Identifier: Apache-2.0 import argparse +import hashlib +from pathlib import Path + +import yaml from polymath_code_standard.checker import CONFIG_DIR, CheckerGroup, Result, check_group +# Where a repo declares the collections and roles its playbooks import. +REQUIREMENTS = Path('ansible/requirements.yml') + +# Where we put them: a cache this hook owns, in the consuming repo but out of the +# way. It carries its own '*' .gitignore so no downstream repo has to add one. +# +# The path is ours to choose because ansible-lint runs `ansible-playbook +# --syntax-check` as a child process that receives exactly the paths exported +# below (plus site-packages). Note ansible-lint logs a `/.ansible/collections` +# entry belonging to its own isolated runtime which it never passes on -- content +# installed there is invisible to syntax-check, which then reports every role as +# "not found". +CACHE_DIR = Path('.polymath-ansible') +COLLECTIONS_DIR = CACHE_DIR / 'collections' +ROLES_DIR = CACHE_DIR / 'roles' + +# Installing means downloading, and cloning from git for our own collection: far +# too slow for every commit. Stamp the requirements digest beside the installed +# tree and re-install only when the file changes. +STAMP = CACHE_DIR / 'requirements-sha256' + @check_group class AnsibleGroup(CheckerGroup): name = 'ansible' def run(self, args: argparse.Namespace) -> list[Result]: - return [ + results = [] + + for result in self._install_requirements(): + results.append(result) + if not result.passed: + # Linting now would bury the real error under a pile of + # syntax-check "role not found" noise. + return results + + results.append( self._check( 'python3', - ['-m', 'ansiblelint', '-v', '--force-color', '-c', CONFIG_DIR / 'ansible-lint.yml'], + [ + '-m', + 'ansiblelint', + '-v', + '--force-color', + # Without this the project root is wherever --config lives (inside + # this package), which misplaces requirements lookup and excludes. + '--project-dir', + '.', + '-c', + CONFIG_DIR / 'ansible-lint.yml', + ], args.files, name='ansible-lint', - env={'ANSIBLE_COLLECTIONS_PATH': 'ansible/collections'}, + env={ + 'ANSIBLE_COLLECTIONS_PATH': str(COLLECTIONS_DIR), + 'ANSIBLE_ROLES_PATH': str(ROLES_DIR), + }, ) + ) + return results + + @classmethod + def _install_requirements(cls) -> list[Result]: + """Install declared content, or return nothing when there is nothing to do. + + ansible-lint does install requirements itself, but only from the locations + ansible_compat hardcodes (requirements.yml, roles/requirements.yml, + collections/requirements.yml, tests/...), resolved against its notion of the + project root -- the directory of the --config file we pass, i.e. a path + inside this package. Neither matches a Polymath repo, so we install here. + """ + if not REQUIREMENTS.is_file(): + return [] + + raw = REQUIREMENTS.read_bytes() + digest = hashlib.sha256(raw).hexdigest() + if STAMP.is_file() and STAMP.read_text().strip() == digest: + return [] + + try: + declared = yaml.safe_load(raw) or {} + except yaml.YAMLError as exc: + return [Result(name='ansible-galaxy', passed=False, output=f'{REQUIREMENTS}: {exc}')] + + cls._make_cache_dir() + + # `collection install` silently skips a roles-only file ("Skipping install, + # no requirements found", exit 0) and vice versa, so dispatch on what is + # actually declared rather than always running both. + installs = [ + ('collection', COLLECTIONS_DIR, declared.get('collections')), + ('role', ROLES_DIR, declared.get('roles')), ] + + results = [] + for kind, dest, entries in installs: + if not entries: + continue + result = cls._check( + 'ansible-galaxy', + [kind, 'install', '-r', str(REQUIREMENTS), '-p', str(dest)], + None, + name='ansible-galaxy', + # `-p` says where to install but not where to look: galaxy decides + # "already installed" from the configured search paths, so anything + # present in the developer's ~/.ansible ends with "Nothing to do" + # and an empty cache that the linter then cannot resolve. + env={'ANSIBLE_COLLECTIONS_PATH': str(COLLECTIONS_DIR), 'ANSIBLE_ROLES_PATH': str(ROLES_DIR)}, + ) + results.append(result) + if not result.passed: + return results + + STAMP.write_text(f'{digest}\n') + return results + + @staticmethod + def _make_cache_dir() -> None: + """Create the cache and make it ignore itself, including on a failed install.""" + CACHE_DIR.mkdir(parents=True, exist_ok=True) + gitignore = CACHE_DIR / '.gitignore' + if not gitignore.is_file(): + gitignore.write_text('*\n') diff --git a/polymath_code_standard/config/ansible-lint.yml b/polymath_code_standard/config/ansible-lint.yml index 9cd2630..06f809e 100644 --- a/polymath_code_standard/config/ansible-lint.yml +++ b/polymath_code_standard/config/ansible-lint.yml @@ -1,2 +1,9 @@ --- profile: production + +exclude_paths: + # The hook's own cache of collections and roles installed from + # ansible/requirements.yml. Third-party (or another repo's) content that a + # consumer cannot fix, and linting it here would fail commits over violations + # in someone else's tree. Keep in sync with CACHE_DIR in checkers/ansible.py. + - .polymath-ansible/ diff --git a/pyproject.toml b/pyproject.toml index aeadec1..82bcef8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,6 +37,12 @@ dev = [ "pytest>=9.0.3", ] +[tool.pytest.ini_options] +markers = [ + # Reaches Ansible Galaxy to install real requirements. Deselect with -m 'not network'. + "network: test requires network access", +] + [project.scripts] polymath_code_standard = "polymath_code_standard.runner:main" polymath_copyright_header = "polymath_code_standard.insert_license:main" diff --git a/test_files/ansible/inventory.yml b/test_files/ansible/inventory.yml new file mode 100644 index 0000000..05b74b2 --- /dev/null +++ b/test_files/ansible/inventory.yml @@ -0,0 +1,5 @@ +--- + +all: + vars: + foo: well hello there diff --git a/test_files/ansible/playbook.yml b/test_files/ansible/playbook.yml new file mode 100644 index 0000000..73a4aef --- /dev/null +++ b/test_files/ansible/playbook.yml @@ -0,0 +1,20 @@ +--- + +- name: Configure some computer + hosts: all + pre_tasks: + - name: Ensure host is reachable + ansible.builtin.ping: + + # From the ansible.posix collection: syntax-check can only resolve this if the + # hook installed requirements.yml somewhere the child process can see. + - name: Enable IP forwarding + ansible.posix.sysctl: + name: net.ipv4.ip_forward + value: '1' + sysctl_set: true + state: present + reload: true + + roles: + - role: geerlingguy.docker diff --git a/test_files/ansible/requirements.yml b/test_files/ansible/requirements.yml new file mode 100644 index 0000000..1b2a58d --- /dev/null +++ b/test_files/ansible/requirements.yml @@ -0,0 +1,7 @@ +--- +# Both kinds are declared on purpose: `ansible-galaxy collection install` silently +# ignores a roles-only file and vice versa, so the hook has to dispatch on each key. +collections: + - name: ansible.posix +roles: + - name: geerlingguy.docker diff --git a/tests/test_runner.py b/tests/test_runner.py index 524ec34..f670082 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -2,6 +2,7 @@ # SPDX-License-Identifier: Apache-2.0 """Smoke tests: verify runner.main dispatches to each checker and runs without error.""" +import hashlib import shutil import uuid from pathlib import Path @@ -10,6 +11,7 @@ from polymath_code_standard import runner from polymath_code_standard.checker import _GROUPS +from polymath_code_standard.checkers import ansible as ansible_checker _PROJECT_ROOT = Path(__file__).parent.parent @@ -165,3 +167,51 @@ def test_ansible(make_file): ) f = make_file('playbook.yml', content) assert runner.main(['ansible', f]) == 0 + + +@pytest.mark.network +def test_ansible_installs_requirements(tmp_path, monkeypatch): + """A downstream repo declaring requirements.yml gets them installed, then linted. + + The fixture playbook imports a role and a module from a collection, neither of + which ansible-lint resolves on its own: it only auto-installs requirements from + paths relative to its own project root, which is this package's config dir. + """ + shutil.copytree(_PROJECT_ROOT / 'test_files' / 'ansible', tmp_path / 'ansible') + monkeypatch.chdir(tmp_path) + + assert runner.main(['ansible', 'ansible/playbook.yml']) == 0 + + # A passing lint already proves the syntax-check child resolved both, but assert + # the layout too, so moving these paths fails loudly instead of silently + # depending on some other collections path that happens to be populated. + assert (tmp_path / ansible_checker.COLLECTIONS_DIR / 'ansible_collections' / 'ansible' / 'posix').is_dir() + assert (tmp_path / ansible_checker.ROLES_DIR / 'geerlingguy.docker').is_dir() + + # The cache hides itself, so consuming repos need no .gitignore edit. + assert (tmp_path / ansible_checker.CACHE_DIR / '.gitignore').read_text().strip() == '*' + + # Second run installs nothing: re-cloning on every commit would be far too slow. + assert ansible_checker.AnsibleGroup._install_requirements() == [] + + +def test_ansible_stamp_tracks_requirements(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / 'ansible').mkdir() + requirements = tmp_path / ansible_checker.REQUIREMENTS + requirements.write_text('---\ncollections:\n - name: ansible.posix\n') + + stamp = tmp_path / ansible_checker.STAMP + stamp.parent.mkdir(parents=True) + stamp.write_text(hashlib.sha256(requirements.read_bytes()).hexdigest()) + assert ansible_checker.AnsibleGroup._install_requirements() == [] + + # Editing requirements.yml invalidates the stamp so the install runs again. + requirements.write_text('---\nroles:\n - name: geerlingguy.docker\n') + assert ansible_checker.AnsibleGroup._install_requirements() != [] + + +def test_ansible_without_requirements_installs_nothing(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + assert ansible_checker.AnsibleGroup._install_requirements() == [] + assert not (tmp_path / ansible_checker.CACHE_DIR).exists() From 09322af4ce6f2a5456adc7cac758ee2402da24b3 Mon Sep 17 00:00:00 2001 From: Emerson Knapp Date: Thu, 13 Aug 2026 16:16:51 -0700 Subject: [PATCH 2/2] Fix up git-sources requirements dependencies Signed-off-by: Emerson Knapp --- polymath_code_standard/checkers/ansible.py | 53 ++++++++++ tests/test_ansible.py | 110 +++++++++++++++++++++ tests/test_runner.py | 50 ---------- 3 files changed, 163 insertions(+), 50 deletions(-) create mode 100644 tests/test_ansible.py diff --git a/polymath_code_standard/checkers/ansible.py b/polymath_code_standard/checkers/ansible.py index 2578094..018d25b 100644 --- a/polymath_code_standard/checkers/ansible.py +++ b/polymath_code_standard/checkers/ansible.py @@ -2,6 +2,7 @@ # SPDX-License-Identifier: Apache-2.0 import argparse import hashlib +import json from pathlib import Path import yaml @@ -121,9 +122,61 @@ def _install_requirements(cls) -> list[Result]: if not result.passed: return results + git_deps = cls._install_git_dependencies(declared.get('collections') or []) + results.extend(git_deps) + if any(not r.passed for r in git_deps): + return results + STAMP.write_text(f'{digest}\n') return results + @classmethod + def _install_git_dependencies(cls, collections: list) -> list[Result]: + """Install the dependencies of git-sourced collections. + + ansible-galaxy resolves dependencies for collections it pulls from a galaxy + server, but not for ones installed from git: those arrive with their + dependencies unmet, and playbooks then fail on modules like + community.general.modprobe. Requiring every consumer to restate the list is + how it silently rots, so read it from the collection we just installed. + + Only one pass is needed: whatever we install here comes from a galaxy server, + so galaxy resolves its dependencies for us. + """ + if not any(cls._is_git_source(entry) for entry in collections): + return [] + + installed = {} + for manifest in sorted(COLLECTIONS_DIR.glob('ansible_collections/*/*/MANIFEST.json')): + try: + info = json.loads(manifest.read_text())['collection_info'] + except (json.JSONDecodeError, KeyError, OSError): + continue + installed[f'{info["namespace"]}.{info["name"]}'] = info.get('dependencies') or {} + + missing = {name: spec for deps in installed.values() for name, spec in deps.items() if name not in installed} + if not missing: + return [] + + # A '*' requirement is expressed by passing the bare name. + targets = [f'{name}:{spec}' if spec and spec != '*' else name for name, spec in sorted(missing.items())] + return [ + cls._check( + 'ansible-galaxy', + ['collection', 'install', *targets, '-p', str(COLLECTIONS_DIR)], + None, + name='ansible-galaxy', + env={'ANSIBLE_COLLECTIONS_PATH': str(COLLECTIONS_DIR), 'ANSIBLE_ROLES_PATH': str(ROLES_DIR)}, + ) + ] + + @staticmethod + def _is_git_source(entry: object) -> bool: + if not isinstance(entry, dict): + return False + name = str(entry.get('name', '')) + return entry.get('type') == 'git' or name.startswith('git@') or name.endswith('.git') + @staticmethod def _make_cache_dir() -> None: """Create the cache and make it ignore itself, including on a failed install.""" diff --git a/tests/test_ansible.py b/tests/test_ansible.py new file mode 100644 index 0000000..20fd0e7 --- /dev/null +++ b/tests/test_ansible.py @@ -0,0 +1,110 @@ +# SPDX-FileCopyrightText: 2026 Polymath Robotics, Inc. +# SPDX-License-Identifier: Apache-2.0 +"""Tests for the ansible checker group. + +Beyond running ansible-lint, this group installs what a repo declares in +ansible/requirements.yml, because ansible-lint will not: it only auto-installs +from paths ansible_compat hardcodes, resolved against its notion of the project +root -- which is the directory of the --config file we pass, i.e. a path inside +this package. +""" + +import hashlib +import json +import shutil +from pathlib import Path + +import pytest + +from polymath_code_standard import runner +from polymath_code_standard.checker import Result +from polymath_code_standard.checkers import ansible as ansible_checker + +_PROJECT_ROOT = Path(__file__).parent.parent + + +def _write_manifest(root: Path, fqcn: str, dependencies: dict) -> None: + """Fake a collection installed by ansible-galaxy, which records deps in MANIFEST.json.""" + namespace, name = fqcn.split('.') + path = root / 'ansible_collections' / namespace / name + path.mkdir(parents=True) + (path / 'MANIFEST.json').write_text( + json.dumps({'collection_info': {'namespace': namespace, 'name': name, 'dependencies': dependencies}}) + ) + + +@pytest.mark.network +def test_ansible_installs_requirements(tmp_path, monkeypatch): + """A downstream repo declaring requirements.yml gets them installed, then linted. + + The fixture playbook imports a role and a module from a collection, neither of + which ansible-lint resolves on its own. + """ + shutil.copytree(_PROJECT_ROOT / 'test_files' / 'ansible', tmp_path / 'ansible') + monkeypatch.chdir(tmp_path) + + assert runner.main(['ansible', 'ansible/playbook.yml']) == 0 + + # A passing lint already proves the syntax-check child resolved both, but assert + # the layout too, so moving these paths fails loudly instead of silently + # depending on some other collections path that happens to be populated. + assert (tmp_path / ansible_checker.COLLECTIONS_DIR / 'ansible_collections' / 'ansible' / 'posix').is_dir() + assert (tmp_path / ansible_checker.ROLES_DIR / 'geerlingguy.docker').is_dir() + + # The cache hides itself, so consuming repos need no .gitignore edit. + assert (tmp_path / ansible_checker.CACHE_DIR / '.gitignore').read_text().strip() == '*' + + # Second run installs nothing: re-cloning on every commit would be far too slow. + assert ansible_checker.AnsibleGroup._install_requirements() == [] + + +def test_ansible_stamp_tracks_requirements(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / 'ansible').mkdir() + requirements = tmp_path / ansible_checker.REQUIREMENTS + requirements.write_text('---\ncollections:\n - name: ansible.posix\n') + + stamp = tmp_path / ansible_checker.STAMP + stamp.parent.mkdir(parents=True) + stamp.write_text(hashlib.sha256(requirements.read_bytes()).hexdigest()) + assert ansible_checker.AnsibleGroup._install_requirements() == [] + + # Editing requirements.yml invalidates the stamp so the install runs again. + requirements.write_text('---\nroles:\n - name: geerlingguy.docker\n') + assert ansible_checker.AnsibleGroup._install_requirements() != [] + + +def test_ansible_without_requirements_installs_nothing(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + assert ansible_checker.AnsibleGroup._install_requirements() == [] + assert not (tmp_path / ansible_checker.CACHE_DIR).exists() + + +def test_ansible_git_dependencies_detected_from_manifest(tmp_path, monkeypatch): + """Unmet dependencies of an installed collection are found, met ones ignored.""" + monkeypatch.chdir(tmp_path) + cache = tmp_path / ansible_checker.COLLECTIONS_DIR + _write_manifest(cache, 'polymath.core', {'community.general': '>=10.4', 'amazon.aws': '*'}) + _write_manifest(cache, 'amazon.aws', {}) + + calls = [] + monkeypatch.setattr( + ansible_checker.AnsibleGroup, + '_check', + classmethod(lambda cls, tool, args, files, **kw: calls.append(args) or Result(name=tool, passed=True)), + ) + + git_entry = [{'name': 'git@github.com:polymathrobotics/polymath_core.git', 'type': 'git'}] + assert ansible_checker.AnsibleGroup._install_git_dependencies(git_entry) == [ + Result(name='ansible-galaxy', passed=True) + ] + + # amazon.aws is already installed; a '*' spec is passed as a bare name. + assert calls == [['collection', 'install', 'community.general:>=10.4', '-p', str(ansible_checker.COLLECTIONS_DIR)]] + + +def test_ansible_registry_only_requirements_skip_dependency_pass(tmp_path, monkeypatch): + """galaxy resolves dependencies itself for non-git sources, so we stay out of it.""" + monkeypatch.chdir(tmp_path) + _write_manifest(tmp_path / ansible_checker.COLLECTIONS_DIR, 'polymath.core', {'community.general': '>=10.4'}) + assert ansible_checker.AnsibleGroup._install_git_dependencies([{'name': 'ansible.posix'}]) == [] diff --git a/tests/test_runner.py b/tests/test_runner.py index f670082..524ec34 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -2,7 +2,6 @@ # SPDX-License-Identifier: Apache-2.0 """Smoke tests: verify runner.main dispatches to each checker and runs without error.""" -import hashlib import shutil import uuid from pathlib import Path @@ -11,7 +10,6 @@ from polymath_code_standard import runner from polymath_code_standard.checker import _GROUPS -from polymath_code_standard.checkers import ansible as ansible_checker _PROJECT_ROOT = Path(__file__).parent.parent @@ -167,51 +165,3 @@ def test_ansible(make_file): ) f = make_file('playbook.yml', content) assert runner.main(['ansible', f]) == 0 - - -@pytest.mark.network -def test_ansible_installs_requirements(tmp_path, monkeypatch): - """A downstream repo declaring requirements.yml gets them installed, then linted. - - The fixture playbook imports a role and a module from a collection, neither of - which ansible-lint resolves on its own: it only auto-installs requirements from - paths relative to its own project root, which is this package's config dir. - """ - shutil.copytree(_PROJECT_ROOT / 'test_files' / 'ansible', tmp_path / 'ansible') - monkeypatch.chdir(tmp_path) - - assert runner.main(['ansible', 'ansible/playbook.yml']) == 0 - - # A passing lint already proves the syntax-check child resolved both, but assert - # the layout too, so moving these paths fails loudly instead of silently - # depending on some other collections path that happens to be populated. - assert (tmp_path / ansible_checker.COLLECTIONS_DIR / 'ansible_collections' / 'ansible' / 'posix').is_dir() - assert (tmp_path / ansible_checker.ROLES_DIR / 'geerlingguy.docker').is_dir() - - # The cache hides itself, so consuming repos need no .gitignore edit. - assert (tmp_path / ansible_checker.CACHE_DIR / '.gitignore').read_text().strip() == '*' - - # Second run installs nothing: re-cloning on every commit would be far too slow. - assert ansible_checker.AnsibleGroup._install_requirements() == [] - - -def test_ansible_stamp_tracks_requirements(tmp_path, monkeypatch): - monkeypatch.chdir(tmp_path) - (tmp_path / 'ansible').mkdir() - requirements = tmp_path / ansible_checker.REQUIREMENTS - requirements.write_text('---\ncollections:\n - name: ansible.posix\n') - - stamp = tmp_path / ansible_checker.STAMP - stamp.parent.mkdir(parents=True) - stamp.write_text(hashlib.sha256(requirements.read_bytes()).hexdigest()) - assert ansible_checker.AnsibleGroup._install_requirements() == [] - - # Editing requirements.yml invalidates the stamp so the install runs again. - requirements.write_text('---\nroles:\n - name: geerlingguy.docker\n') - assert ansible_checker.AnsibleGroup._install_requirements() != [] - - -def test_ansible_without_requirements_installs_nothing(tmp_path, monkeypatch): - monkeypatch.chdir(tmp_path) - assert ansible_checker.AnsibleGroup._install_requirements() == [] - assert not (tmp_path / ansible_checker.CACHE_DIR).exists()