From 5aeeb094ebe374576dfcaecff6a72b256d2c997f Mon Sep 17 00:00:00 2001 From: sjat Date: Sat, 6 Jun 2026 15:12:48 +0200 Subject: [PATCH] feat(tags): enforce role imports carry their role-name tag Adds role_tag_problems() to check-tags.py: every role imported in a play's roles: block must carry its own role name as a tag (extra tags allowed; templated role names skipped). Wires the check into main() so make lint catches violations. 6 new unit tests (29 total, all passing). Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 2 +- docs/decisions/019-tagging.md | 3 +- scripts/check-tags.py | 85 ++++++++++++++++++++++++++++++----- tests/test_check_tags.py | 58 ++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 14 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 10f70ce..115d2b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,7 +55,7 @@ Full design rationale: `docs/decisions/` (Ansible inherits it to every task). Tag a task/block with a concern tag from the approved list (`tests/tags.yml`) only where it genuinely belongs to that concern — don't invent tags or tag for tagging's sake. Target one axis at a time (role/service - *or* concern; tags are union/OR, never intersected). `make lint` enforces the vocabulary. + *or* concern; tags are union/OR, never intersected). `make lint` enforces the vocabulary and that each role import carries its role-name tag. - **Handlers**: use `listen:` topic strings, not direct name references - **Variables**: `rolename__varname` double-underscore namespace for role defaults - **No inline vars in playbooks**: use `group_vars/` or `host_vars/` only diff --git a/docs/decisions/019-tagging.md b/docs/decisions/019-tagging.md index 31c19d9..0ac4201 100644 --- a/docs/decisions/019-tagging.md +++ b/docs/decisions/019-tagging.md @@ -91,6 +91,7 @@ opt-in/playbook tags. `scripts/check-tags.py` (run by `make lint`, covered by `tests/test_check_tags.py`) scans `roles/` and `playbooks/` and fails on any tag outside `{role directory names} ∪ {tests/tags.yml entries}`. Molecule scenario files (`roles/*/molecule/**`) are excluded from the scan — they are test orchestration, not the production run-targeting surface this standard governs. +It also checks that every role imported in a play's `roles:` block carries its own role name as a tag (additional tags are allowed). ## Extending the vocabulary @@ -104,7 +105,7 @@ leaves a paper trail. - Targeted runs are predictable: only two kinds of tags exist, one of them mechanical. - Over-tagging is structurally resisted (closed list + lint enforcement). - Intersection targeting is unavailable by design. -- Authors must keep role tags = role names. The linter enforces the *vocabulary* (every tag must be a known role name or an approved tag); the role-tag-equals-role-name rule itself is a convention the linter does not separately check. +- Authors must keep role tags = role names. `make lint` enforces both the *vocabulary* (every tag is a known role name or approved tag) and that each role import in a `roles:` block carries its own role-name tag (extra tags allowed). ## Related diff --git a/scripts/check-tags.py b/scripts/check-tags.py index e967f8a..3e02bd1 100644 --- a/scripts/check-tags.py +++ b/scripts/check-tags.py @@ -7,6 +7,9 @@ Allowed set = {role directory names under roles/} ∪ {concerns, special, opt_in playbooks from tests/tags.yml}. Templated tags (containing "{{") are skipped — they can't be statically validated. +It also checks that every role imported in a play's roles: block carries its own +role name as a tag (extra tags allowed); ADR-019. + Usage: python3 scripts/check-tags.py Exit 0 = all tags allowed; exit 1 = unknown tag(s) found. """ @@ -76,6 +79,47 @@ def scan_text(text): return found +def _role_entry(entry): + """Return (role_name, tags_set) for a roles: list entry, or (None, set()) to skip.""" + if isinstance(entry, str): + return (entry if _static_str(entry) else None, set()) + if isinstance(entry, dict): + name = entry.get("role") + if not _static_str(name): + return (None, set()) + tags_value = entry.get("tags") + tags = set() + if _static_str(tags_value): + tags.add(tags_value) + elif isinstance(tags_value, list): + tags.update(t for t in tags_value if _static_str(t)) + return (name, tags) + return (None, set()) + + +def _walk_roles(node, problems): + if isinstance(node, dict): + roles = node.get("roles") + if isinstance(roles, list): + for entry in roles: + name, tags = _role_entry(entry) + if name is not None and name not in tags: + problems.append((name, sorted(tags))) + for value in node.values(): + _walk_roles(value, problems) + elif isinstance(node, list): + for item in node: + _walk_roles(item, problems) + + +def role_tag_problems(text): + """Role imports (roles: blocks) whose tags don't include their own role name.""" + problems = [] + for doc in yaml.load_all(text, Loader=_IgnoreUnknownTags): + _walk_roles(doc, problems) + return problems + + def iter_yaml_files(repo=REPO, scan_dirs=SCAN_DIRS): for name in scan_dirs: base = repo / name @@ -96,25 +140,42 @@ def find_violations(used, allowed): def main(): allowed = load_vocab() | role_names() - violations = [] + vocab_violations = [] + role_tag_issues = [] for path in iter_yaml_files(): + text = path.read_text() try: - used = scan_text(path.read_text()) + used = scan_text(text) except yaml.YAMLError as exc: print(f"warning: could not parse {path}: {exc}", file=sys.stderr) continue + rel = path.relative_to(REPO) for tag in find_violations(used, allowed): - violations.append((path.relative_to(REPO), tag)) + vocab_violations.append((rel, tag)) + for name, tags in role_tag_problems(text): + role_tag_issues.append((rel, name, tags)) - if violations: - print( - "error: Ansible tag(s) not in tests/tags.yml or role names " - "(see docs/decisions/019-tagging.md):", - file=sys.stderr, - ) - for relpath, tag in violations: - print(f" {relpath}: '{tag}'", file=sys.stderr) - print(f"\nallowed: {', '.join(sorted(allowed))}", file=sys.stderr) + if vocab_violations or role_tag_issues: + if vocab_violations: + print( + "error: Ansible tag(s) not in tests/tags.yml or role names " + "(see docs/decisions/019-tagging.md):", + file=sys.stderr, + ) + for relpath, tag in vocab_violations: + print(f" {relpath}: '{tag}'", file=sys.stderr) + print(f"\nallowed: {', '.join(sorted(allowed))}", file=sys.stderr) + if role_tag_issues: + print( + "error: role import(s) missing their role-name tag " + "(ADR-019: import each role with tags: []):", + file=sys.stderr, + ) + for relpath, name, tags in role_tag_issues: + print( + f" {relpath}: role '{name}' has tags {tags}, must include '{name}'", + file=sys.stderr, + ) sys.exit(1) print(f"check-tags: OK ({len(allowed)} tags allowed across {len(SCAN_DIRS)} dirs)") diff --git a/tests/test_check_tags.py b/tests/test_check_tags.py index e288cf6..d82d134 100644 --- a/tests/test_check_tags.py +++ b/tests/test_check_tags.py @@ -83,3 +83,61 @@ def test_iter_yaml_files_skips_molecule(tmp_path): names = [p.name for p in found] assert "main.yml" in names assert "verify.yml" not in names + + +def test_role_tag_problems_flags_missing_name(): + text = """ +- hosts: all + roles: + - role: base + tags: [firewall] +""" + assert ct.role_tag_problems(text) == [("base", ["firewall"])] + + +def test_role_tag_problems_allows_extra_tags(): + text = """ +- hosts: all + roles: + - role: base + tags: [base, never] +""" + assert ct.role_tag_problems(text) == [] + + +def test_role_tag_problems_flags_bare_dict_import(): + text = """ +- hosts: all + roles: + - role: base +""" + assert ct.role_tag_problems(text) == [("base", [])] + + +def test_role_tag_problems_flags_string_form(): + text = """ +- hosts: all + roles: + - base +""" + assert ct.role_tag_problems(text) == [("base", [])] + + +def test_role_tag_problems_ok_when_name_present(): + text = """ +- hosts: all + roles: + - role: docker_host + tags: [docker_host] +""" + assert ct.role_tag_problems(text) == [] + + +def test_role_tag_problems_skips_templated_role(): + text = """ +- hosts: all + roles: + - role: "{{ dynamic }}" + tags: [foo] +""" + assert ct.role_tag_problems(text) == []