From 2d073a2096e6fe5cc7b36d1dc210b3874f5ba379 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 29 Jun 2026 17:18:40 +1200 Subject: [PATCH 01/19] ci: add check that codeowners are provided and unorphaned --- .../adding-a-new-library.md | 1 - .../migrating-a-library.md | 1 - .github/workflows/ci.yaml | 10 ++ .scripts/check_codeowners.py | 147 ++++++++++++++++++ .scripts/just.py | 7 + .scripts/tests/test_check_codeowners.py | 123 +++++++++++++++ CODEOWNERS | 52 +++++++ justfile | 3 + 8 files changed, 342 insertions(+), 2 deletions(-) create mode 100755 .scripts/check_codeowners.py create mode 100644 .scripts/tests/test_check_codeowners.py diff --git a/.github/PULL_REQUEST_TEMPLATE/adding-a-new-library.md b/.github/PULL_REQUEST_TEMPLATE/adding-a-new-library.md index de0dec2d2..867d8dc0b 100644 --- a/.github/PULL_REQUEST_TEMPLATE/adding-a-new-library.md +++ b/.github/PULL_REQUEST_TEMPLATE/adding-a-new-library.md @@ -33,7 +33,6 @@ Package: Repository metadata: - [ ] `.docs/reference/libs.yaml` updated with a new entry. -- [ ] `CODEOWNERS` updated with a `//` entry for the owning team. Tests and docs: - [ ] Unit tests added, plus functional and integration tests as appropriate. diff --git a/.github/PULL_REQUEST_TEMPLATE/migrating-a-library.md b/.github/PULL_REQUEST_TEMPLATE/migrating-a-library.md index e0d9c34d5..bca7e9f34 100644 --- a/.github/PULL_REQUEST_TEMPLATE/migrating-a-library.md +++ b/.github/PULL_REQUEST_TEMPLATE/migrating-a-library.md @@ -41,7 +41,6 @@ Package: Repository metadata: - [ ] `.docs/reference/libs.yaml` updated with entries for new and old libs. -- [ ] `CODEOWNERS` updated with a `//` entry for the owning team. Tests and docs: - [ ] Unit tests migrated, plus functional and integration tests as appropriate. diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index cdeb9ef37..1d6dcb9e8 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -125,6 +125,16 @@ jobs: uvx --from rust-just just interfaces-json git diff --exit-code + codeowners: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + with: + persist-credentials: false + - uses: astral-sh/setup-uv@v7 + - name: Ensure every package and interface has a CODEOWNERS entry, and no entries are orphaned + run: uvx --from rust-just just check-codeowners + meta: # tests for the repository's own CI tooling and helper scripts runs-on: ubuntu-latest steps: diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py new file mode 100755 index 000000000..4ea595fc2 --- /dev/null +++ b/.scripts/check_codeowners.py @@ -0,0 +1,147 @@ +#!/usr/bin/env -S uv run --script --no-project + +# /// script +# requires-python = ">=3.12" +# dependencies = [] +# /// + +# Copyright 2026 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Validate the repository's CODEOWNERS file. + +Two checks are performed: + +1. Every package and interface directory has an explicit code owner, rather than + falling back to the repository maintainers via the catch-all `*` entry. +2. Every path-based CODEOWNERS entry corresponds to a real path in the repository, + so renaming or removing a directory can't leave behind an orphan entry. + +Exit with success (0) if both checks pass, otherwise print the problems to stdout +and exit with failure (the number of problems found). +""" + +from __future__ import annotations + +import json +import pathlib +import subprocess +import sys +import typing + +if typing.TYPE_CHECKING: + from collections.abc import Iterable + +# `.scripts/check_codeowners.py` -> repo root is two parents up. +REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent +CODEOWNERS = REPO_ROOT / 'CODEOWNERS' +LS = pathlib.Path(__file__).resolve().parent / 'ls.py' + + +def main() -> int: + """Run both CODEOWNERS checks, printing any problems and returning the problem count.""" + entries = parse_codeowners(CODEOWNERS.read_text()) + directories = [*_ls('packages'), *_ls('interfaces')] + problems = 0 + for directory in find_unowned_dirs(directories, entries): + print(f'No explicit CODEOWNERS entry for: /{directory}/') + problems += 1 + for pattern in find_orphan_entries(entries, REPO_ROOT): + print(f'CODEOWNERS entry points at a missing path: {pattern}') + problems += 1 + if problems == 0: + print('All packages and interfaces have a CODEOWNERS entry, and no entries are orphaned.') + return problems + + +class Entry(typing.NamedTuple): + """A single CODEOWNERS entry: a path pattern and its owners.""" + + pattern: str + owners: tuple[str, ...] + + +def parse_codeowners(text: str) -> list[Entry]: + """Parse CODEOWNERS file contents into a list of `Entry`s, ignoring comments and blanks.""" + entries: list[Entry] = [] + for line in text.splitlines(): + line = line.split('#', 1)[0].strip() + if not line: + continue + pattern, *owners = line.split() + entries.append(Entry(pattern=pattern, owners=tuple(owners))) + return entries + + +def _ls(category: str) -> list[str]: + """Return the repo-relative paths of all `packages` or `interfaces`, excluding testing. + + Example and namespace-placeholder directories are included: they exist in the repo + long-term, so they get an explicit CODEOWNERS entry like any other directory. Testing + packages are excluded, as they're always children of an existing library and so are + already covered by that library's entry. + """ + cmd = [str(LS), category, '--exclude-testing'] + return json.loads(subprocess.check_output(cmd)) + + +def find_unowned_dirs(directories: Iterable[str], entries: Iterable[Entry]) -> list[str]: + """Return directories without an explicit (non catch-all) CODEOWNERS entry with an owner. + + A directory is considered explicitly owned if some entry with at least one owner points + at the directory itself or at a path inside it. This covers both whole-directory entries + (e.g. `/apt/`) and split entries (e.g. `/interfaces/foo/interface/` plus + `/interfaces/foo/ruff.toml`), without treating the catch-all `*` fallback as ownership. + """ + owned_targets = [ + target + for entry in entries + if entry.owners + if (target := entry_target(entry.pattern)) is not None + ] + unowned: list[str] = [] + for directory in directories: + dir_path = pathlib.PurePosixPath(directory.strip('/')) + if not any(target == dir_path or dir_path in target.parents for target in owned_targets): + unowned.append(directory) + return unowned + + +def find_orphan_entries(entries: Iterable[Entry], root: pathlib.Path) -> list[str]: + """Return the patterns of path-based entries that don't point at an existing path.""" + orphans: list[str] = [] + for entry in entries: + target = entry_target(entry.pattern) + if target is None: + continue + if not (root / target).exists(): + orphans.append(entry.pattern) + return orphans + + +def entry_target(pattern: str) -> pathlib.PurePosixPath | None: + """Return the repo-relative path that an anchored CODEOWNERS path pattern points to. + + Returns `None` for non-path patterns (those containing glob wildcards, or the + catch-all `*`), which aren't validated as real paths. + """ + if any(char in pattern for char in '*?[]'): + return None + # CODEOWNERS paths are anchored to the repo root with a leading slash, and a + # trailing slash just means "this is a directory"; neither affects the path. + return pathlib.PurePosixPath(pattern.strip('/')) + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/.scripts/just.py b/.scripts/just.py index 5724276f1..6bac603c7 100755 --- a/.scripts/just.py +++ b/.scripts/just.py @@ -464,6 +464,13 @@ def interfaces_json(argv: list[str]) -> int: return 0 +@_register +def check_codeowners(argv: list[str]) -> int: + """Check every package and interface has a CODEOWNERS entry, and no entries are orphaned.""" + _parser(check_codeowners).parse_args(argv) # supports `-h` + return _run(['.scripts/check_codeowners.py'], check=False) + + @_register def _scripts_unit(argv: list[str]) -> int: """Run the unit tests for the repository tooling in `.scripts/`.""" diff --git a/.scripts/tests/test_check_codeowners.py b/.scripts/tests/test_check_codeowners.py new file mode 100644 index 000000000..35a1c3506 --- /dev/null +++ b/.scripts/tests/test_check_codeowners.py @@ -0,0 +1,123 @@ +# Copyright 2026 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# ruff: noqa: D101, D102, D103 (test docstrings) + +"""Unit tests for the check_codeowners script.""" + +import importlib +import pathlib + +import pytest + +check_codeowners = importlib.import_module('check_codeowners') +Entry = check_codeowners.Entry + + +class TestParseCodeowners: + def test_ignores_comments_and_blanks(self): + text = '# a comment\n\n/apt/ @canonical/team # trailing comment\n' + assert check_codeowners.parse_codeowners(text) == [ + Entry(pattern='/apt/', owners=('@canonical/team',)) + ] + + def test_multiple_owners(self): + text = '/foo/ @canonical/one @canonical/two\n' + assert check_codeowners.parse_codeowners(text) == [ + Entry(pattern='/foo/', owners=('@canonical/one', '@canonical/two')) + ] + + def test_entry_without_owner(self): + text = '/interfaces/index.json\n' + assert check_codeowners.parse_codeowners(text) == [ + Entry(pattern='/interfaces/index.json', owners=()) + ] + + def test_empty(self): + assert check_codeowners.parse_codeowners('') == [] + + +class TestEntryTarget: + @pytest.mark.parametrize( + ('pattern', 'expected'), + [ + ('/apt/', 'apt'), + ('/apt', 'apt'), + ('/interfaces/foo/interface/', 'interfaces/foo/interface'), + ('/interfaces/foo/ruff.toml', 'interfaces/foo/ruff.toml'), + ], + ) + def test_path_patterns(self, pattern: str, expected: str): + assert check_codeowners.entry_target(pattern) == pathlib.PurePosixPath(expected) + + @pytest.mark.parametrize('pattern', ['*', '/*.md', '/foo/*', '/foo/?ar', '/foo/[abc]']) + def test_glob_patterns_are_not_targets(self, pattern: str): + assert check_codeowners.entry_target(pattern) is None + + +class TestFindOrphanEntries: + def test_existing_paths_are_not_orphans(self, tmp_path: pathlib.Path): + (tmp_path / 'apt').mkdir() + (tmp_path / 'interfaces' / 'foo').mkdir(parents=True) + (tmp_path / 'interfaces' / 'foo' / 'ruff.toml').touch() + entries = [ + Entry('/apt/', ('@canonical/team',)), + Entry('/interfaces/foo/', ('@canonical/team',)), + Entry('/interfaces/foo/ruff.toml', ('@canonical/team',)), + ] + assert check_codeowners.find_orphan_entries(entries, tmp_path) == [] + + def test_missing_path_is_orphan(self, tmp_path: pathlib.Path): + (tmp_path / 'apt').mkdir() + entries = [ + Entry('/apt/', ('@canonical/team',)), + Entry('/gone/', ('@canonical/team',)), + ] + assert check_codeowners.find_orphan_entries(entries, tmp_path) == ['/gone/'] + + def test_glob_entries_are_ignored(self, tmp_path: pathlib.Path): + entries = [Entry('*', ('@canonical/team',)), Entry('/*.md', ('@canonical/team',))] + assert check_codeowners.find_orphan_entries(entries, tmp_path) == [] + + +class TestFindUnownedDirs: + def test_whole_dir_entry_is_owned(self): + entries = [Entry('/apt/', ('@canonical/team',))] + assert check_codeowners.find_unowned_dirs(['apt'], entries) == [] + + def test_entry_inside_dir_counts_as_owned(self): + # Split interface entries: the dir itself has no direct entry, but content does. + entries = [ + Entry('/interfaces/foo/interface/', ('@canonical/team',)), + Entry('/interfaces/foo/ruff.toml', ('@canonical/team',)), + ] + assert check_codeowners.find_unowned_dirs(['interfaces/foo'], entries) == [] + + def test_catch_all_does_not_count_as_owned(self): + entries = [Entry('*', ('@canonical/maintainers',))] + assert check_codeowners.find_unowned_dirs(['apt'], entries) == ['apt'] + + def test_entry_without_owner_does_not_count(self): + entries = [Entry('/apt/', ())] + assert check_codeowners.find_unowned_dirs(['apt'], entries) == ['apt'] + + def test_parent_dir_entry_does_not_own_child(self): + # `/interfaces/` (the fallback) must not be treated as owning a specific interface. + entries = [Entry('/interfaces/', ('@canonical/maintainers',))] + unowned = check_codeowners.find_unowned_dirs(['interfaces/foo'], entries) + assert unowned == ['interfaces/foo'] + + def test_trailing_slash_directory_argument(self): + entries = [Entry('/apt/', ('@canonical/team',))] + assert check_codeowners.find_unowned_dirs(['/apt/'], entries) == [] diff --git a/CODEOWNERS b/CODEOWNERS index 01e74cc9a..2a7de4b2d 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -22,6 +22,11 @@ /.package/ @canonical/charmlibs-maintainers /interfaces/.package/ @canonical/charmlibs-maintainers +# generated examples (kept in sync with the template by CI) +/.example/ @canonical/charmlibs-maintainers +/.tutorial/ @canonical/charmlibs-maintainers +/interfaces/.example/ @canonical/charmlibs-maintainers + # interfaces/index.json doesn't require CODEOWNERS review as its contents are validated in CI /interfaces/index.json @@ -82,6 +87,41 @@ /interfaces/filesystem_info/interface/ @canonical/hpc-team /interfaces/filesystem_info/ruff.toml @canonical/hpc-team +# telco interfaces (fiveg_*, ip_router, sdcore_*) +# The telco team was disbanded, so these are permanently owned by charmlibs-maintainers. + +# /interfaces/fiveg_core_gnb/ +/interfaces/fiveg_core_gnb/interface/ @canonical/charmlibs-maintainers +/interfaces/fiveg_core_gnb/ruff.toml @canonical/charmlibs-maintainers + +# /interfaces/fiveg_f1/ +/interfaces/fiveg_f1/interface/ @canonical/charmlibs-maintainers +/interfaces/fiveg_f1/ruff.toml @canonical/charmlibs-maintainers + +# /interfaces/fiveg_gnb_identity/ +/interfaces/fiveg_gnb_identity/interface/ @canonical/charmlibs-maintainers +/interfaces/fiveg_gnb_identity/ruff.toml @canonical/charmlibs-maintainers + +# /interfaces/fiveg_n2/ +/interfaces/fiveg_n2/interface/ @canonical/charmlibs-maintainers +/interfaces/fiveg_n2/ruff.toml @canonical/charmlibs-maintainers + +# /interfaces/fiveg_n3/ +/interfaces/fiveg_n3/interface/ @canonical/charmlibs-maintainers +/interfaces/fiveg_n3/ruff.toml @canonical/charmlibs-maintainers + +# /interfaces/fiveg_n4/ +/interfaces/fiveg_n4/interface/ @canonical/charmlibs-maintainers +/interfaces/fiveg_n4/ruff.toml @canonical/charmlibs-maintainers + +# /interfaces/fiveg_nrf/ +/interfaces/fiveg_nrf/interface/ @canonical/charmlibs-maintainers +/interfaces/fiveg_nrf/ruff.toml @canonical/charmlibs-maintainers + +# /interfaces/fiveg_rfsim/ +/interfaces/fiveg_rfsim/interface/ @canonical/charmlibs-maintainers +/interfaces/fiveg_rfsim/ruff.toml @canonical/charmlibs-maintainers + # /interfaces/forward_auth/ /interfaces/forward_auth/interface/ @canonical/identity /interfaces/forward_auth/ruff.toml @canonical/identity @@ -114,6 +154,10 @@ /interfaces/hydra_endpoints/interface/ @canonical/identity /interfaces/hydra_endpoints/ruff.toml @canonical/identity +# /interfaces/ip_router/ (telco, owned by charmlibs-maintainers -- see note above) +/interfaces/ip_router/interface/ @canonical/charmlibs-maintainers +/interfaces/ip_router/ruff.toml @canonical/charmlibs-maintainers + # /interfaces/jwt/ /interfaces/jwt/interface/ @canonical/data /interfaces/jwt/ruff.toml @canonical/data @@ -221,6 +265,14 @@ /interfaces/saml/interface/ @canonical/platform-engineering /interfaces/saml/ruff.toml @canonical/platform-engineering +# /interfaces/sdcore_config/ (telco, owned by charmlibs-maintainers -- see note above) +/interfaces/sdcore_config/interface/ @canonical/charmlibs-maintainers +/interfaces/sdcore_config/ruff.toml @canonical/charmlibs-maintainers + +# /interfaces/sdcore_management/ (telco, owned by charmlibs-maintainers -- see note above) +/interfaces/sdcore_management/interface/ @canonical/charmlibs-maintainers +/interfaces/sdcore_management/ruff.toml @canonical/charmlibs-maintainers + # /interfaces/sloth/ /interfaces/sloth/ @canonical/tracing-and-profiling diff --git a/justfile b/justfile index b45b8cf16..d55255d53 100644 --- a/justfile +++ b/justfile @@ -55,6 +55,9 @@ integration-machine *args: interfaces-json *args: @.scripts/just.py interfaces-json "$@" +check-codeowners *args: + @.scripts/just.py check-codeowners "$@" + _scripts-unit *args: @.scripts/just.py scripts-unit "$@" From 12e292dfe520d088310496ff1499ba8d51b100f5 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 29 Jun 2026 18:11:28 +1200 Subject: [PATCH 02/19] ci: refactor script --- .scripts/check_codeowners.py | 114 +++++++++++++++++------- .scripts/tests/test_check_codeowners.py | 98 ++++++++++++++++---- CODEOWNERS | 9 ++ 3 files changed, 169 insertions(+), 52 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index 4ea595fc2..17ad38339 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -21,12 +21,15 @@ """Validate the repository's CODEOWNERS file. -Two checks are performed: +Two checks are performed against every top-level path and every immediate child of `interfaces/`: -1. Every package and interface directory has an explicit code owner, rather than - falling back to the repository maintainers via the catch-all `*` entry. -2. Every path-based CODEOWNERS entry corresponds to a real path in the repository, - so renaming or removing a directory can't leave behind an orphan entry. +1. Each path is owned, meaning either the path itself has a CODEOWNERS entry with an owner, or + (for a directory) every one of its immediate children is owned. This ensures ownership isn't + left to fall through to the repository maintainers via the catch-all `*` entry, while still + allowing a directory to be covered by per-child entries (such as an interface's `interface/` + plus `ruff.toml`). +2. Every path-based CODEOWNERS entry corresponds to a real path in the repository, so renaming + or removing a directory can't leave behind an orphan entry. Exit with success (0) if both checks pass, otherwise print the problems to stdout and exit with failure (the number of problems found). @@ -34,7 +37,6 @@ from __future__ import annotations -import json import pathlib import subprocess import sys @@ -46,22 +48,21 @@ # `.scripts/check_codeowners.py` -> repo root is two parents up. REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent CODEOWNERS = REPO_ROOT / 'CODEOWNERS' -LS = pathlib.Path(__file__).resolve().parent / 'ls.py' def main() -> int: """Run both CODEOWNERS checks, printing any problems and returning the problem count.""" entries = parse_codeowners(CODEOWNERS.read_text()) - directories = [*_ls('packages'), *_ls('interfaces')] + paths = build_paths(tracked_files(REPO_ROOT)) problems = 0 - for directory in find_unowned_dirs(directories, entries): - print(f'No explicit CODEOWNERS entry for: /{directory}/') + for path in find_unowned_dirs(entries, paths): + print(f'No explicit CODEOWNERS entry for: /{path}') problems += 1 for pattern in find_orphan_entries(entries, REPO_ROOT): print(f'CODEOWNERS entry points at a missing path: {pattern}') problems += 1 if problems == 0: - print('All packages and interfaces have a CODEOWNERS entry, and no entries are orphaned.') + print('Every path has a CODEOWNERS owner, and no entries are orphaned.') return problems @@ -72,6 +73,18 @@ class Entry(typing.NamedTuple): owners: tuple[str, ...] +class Path(typing.NamedTuple): + """A repo path to check, with its immediate children. + + `path` is relative to the repo root; directory paths end with `/`. `child_paths` are the + immediate children (also repo-relative, directories ending with `/`), and is empty for + files and for directories with no tracked children. + """ + + path: str + child_paths: tuple[str, ...] + + def parse_codeowners(text: str) -> list[Entry]: """Parse CODEOWNERS file contents into a list of `Entry`s, ignoring comments and blanks.""" entries: list[Entry] = [] @@ -84,37 +97,70 @@ def parse_codeowners(text: str) -> list[Entry]: return entries -def _ls(category: str) -> list[str]: - """Return the repo-relative paths of all `packages` or `interfaces`, excluding testing. +def tracked_files(root: pathlib.Path) -> list[str]: + """Return the repo-relative POSIX paths of all files tracked by git in `root`. - Example and namespace-placeholder directories are included: they exist in the repo - long-term, so they get an explicit CODEOWNERS entry like any other directory. Testing - packages are excluded, as they're always children of an existing library and so are - already covered by that library's entry. + Only tracked files are returned, so untracked and ignored paths (e.g. `.venv/`, caches) + don't need a CODEOWNERS entry. """ - cmd = [str(LS), category, '--exclude-testing'] - return json.loads(subprocess.check_output(cmd)) + output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=root, text=True) + return [file for file in output.split('\0') if file] -def find_unowned_dirs(directories: Iterable[str], entries: Iterable[Entry]) -> list[str]: - """Return directories without an explicit (non catch-all) CODEOWNERS entry with an owner. +def build_paths(files: Iterable[str]) -> list[Path]: + """Build the `Path`s to check from a list of repo-relative tracked file paths. - A directory is considered explicitly owned if some entry with at least one owner points - at the directory itself or at a path inside it. This covers both whole-directory entries - (e.g. `/apt/`) and split entries (e.g. `/interfaces/foo/interface/` plus - `/interfaces/foo/ruff.toml`), without treating the catch-all `*` fallback as ownership. + Returns an item for every top-level entry and every immediate child of `interfaces/`. A + directory's `path` ends with `/` and its `child_paths` list its immediate children (also + with a trailing `/` for directories); files have an empty `child_paths`. """ - owned_targets = [ - target - for entry in entries - if entry.owners - if (target := entry_target(entry.pattern)) is not None + children: dict[str, set[str]] = {} # parent prefix -> immediate child names + is_dir: dict[str, bool] = {} # path component prefix -> whether it has children + for file in files: + parts = file.split('/') + for depth in range(len(parts)): + parent = '/'.join(parts[:depth]) + name = parts[depth] + children.setdefault(parent, set()).add(name) + is_dir[f'{parent}/{name}' if parent else name] = depth < len(parts) - 1 + + def render(prefix: str, name: str) -> str: + path = f'{prefix}/{name}' if prefix else name + return f'{path}/' if is_dir[path] else path + + def child_paths(prefix: str, name: str) -> tuple[str, ...]: + path = f'{prefix}/{name}' if prefix else name + return tuple(sorted(render(path, child) for child in children.get(path, ()))) + + paths = [Path(render('', name), child_paths('', name)) for name in children.get('', ())] + paths += [ + Path(render('interfaces', name), child_paths('interfaces', name)) + for name in children.get('interfaces', ()) ] + return sorted(paths) + + +def find_unowned_dirs(entries: Iterable[Entry], paths: Iterable[Path]) -> list[str]: + """Return the paths without a CODEOWNERS entry, directly or via all their children. + + A path passes if it has its own entry, or (for a directory) every one of its immediate + children has its own entry. An entry without an owner counts: such a path is explicitly + disowned (like `interfaces/index.json`), which is a deliberate decision. This check is not + recursive, so a grandchild entry never satisfies a child, and the catch-all `*` entry isn't a + path entry, so it never stands in for an explicit one. + """ + entry_targets = {target for entry in entries if (target := entry_target(entry.pattern))} + + def has_entry(path: str) -> bool: + return pathlib.PurePosixPath(path) in entry_targets + unowned: list[str] = [] - for directory in directories: - dir_path = pathlib.PurePosixPath(directory.strip('/')) - if not any(target == dir_path or dir_path in target.parents for target in owned_targets): - unowned.append(directory) + for item in paths: + if has_entry(item.path): + continue + if item.child_paths and all(has_entry(child) for child in item.child_paths): + continue + unowned.append(item.path) return unowned diff --git a/.scripts/tests/test_check_codeowners.py b/.scripts/tests/test_check_codeowners.py index 35a1c3506..45f7174fc 100644 --- a/.scripts/tests/test_check_codeowners.py +++ b/.scripts/tests/test_check_codeowners.py @@ -23,6 +23,7 @@ check_codeowners = importlib.import_module('check_codeowners') Entry = check_codeowners.Entry +Path = check_codeowners.Path class TestParseCodeowners: @@ -91,33 +92,94 @@ def test_glob_entries_are_ignored(self, tmp_path: pathlib.Path): assert check_codeowners.find_orphan_entries(entries, tmp_path) == [] +class TestBuildPaths: + def test_top_level_and_interface_children(self): + files = [ + 'README.md', + 'apt/pyproject.toml', + 'apt/src/__init__.py', + 'interfaces/index.json', + 'interfaces/foo/ruff.toml', + 'interfaces/foo/interface/v0/schema.py', + ] + assert check_codeowners.build_paths(files) == [ + Path('README.md', ()), + Path('apt/', ('apt/pyproject.toml', 'apt/src/')), + Path('interfaces/', ('interfaces/foo/', 'interfaces/index.json')), + Path('interfaces/foo/', ('interfaces/foo/interface/', 'interfaces/foo/ruff.toml')), + Path('interfaces/index.json', ()), + ] + + def test_directories_end_with_slash(self): + [apt] = check_codeowners.build_paths(['apt/x.py']) + assert apt.path == 'apt/' + + def test_files_have_no_children(self): + [readme] = check_codeowners.build_paths(['README.md']) + assert readme == Path('README.md', ()) + + def test_empty(self): + assert check_codeowners.build_paths([]) == [] + + class TestFindUnownedDirs: - def test_whole_dir_entry_is_owned(self): + def test_path_with_own_entry_passes(self): entries = [Entry('/apt/', ('@canonical/team',))] - assert check_codeowners.find_unowned_dirs(['apt'], entries) == [] + paths = [Path('apt/', ('apt/src/',))] + assert check_codeowners.find_unowned_dirs(entries, paths) == [] - def test_entry_inside_dir_counts_as_owned(self): - # Split interface entries: the dir itself has no direct entry, but content does. + def test_all_children_owned_passes(self): + # Split interface entries: the dir itself has no entry, but each child does. entries = [ Entry('/interfaces/foo/interface/', ('@canonical/team',)), Entry('/interfaces/foo/ruff.toml', ('@canonical/team',)), ] - assert check_codeowners.find_unowned_dirs(['interfaces/foo'], entries) == [] - - def test_catch_all_does_not_count_as_owned(self): + children = ('interfaces/foo/interface/', 'interfaces/foo/ruff.toml') + paths = [Path('interfaces/foo/', children)] + assert check_codeowners.find_unowned_dirs(entries, paths) == [] + + def test_all_children_disowned_passes(self): + # Children with ownerless entries are explicitly disowned, which satisfies the parent. + entries = [Entry('/foo/a', ()), Entry('/foo/b', ())] + paths = [Path('foo/', ('foo/a', 'foo/b'))] + assert check_codeowners.find_unowned_dirs(entries, paths) == [] + + def test_some_children_unowned_is_unowned(self): + entries = [Entry('/interfaces/foo/interface/', ('@canonical/team',))] + children = ('interfaces/foo/interface/', 'interfaces/foo/ruff.toml') + paths = [Path('interfaces/foo/', children)] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['interfaces/foo/'] + + def test_disowned_path_passes(self): + # An ownerless entry for the path itself counts (e.g. interfaces/index.json). + entries = [Entry('/interfaces/index.json', ())] + paths = [Path('interfaces/index.json', ())] + assert check_codeowners.find_unowned_dirs(entries, paths) == [] + + def test_rule_is_not_recursive(self): + # A grandchild entry must NOT satisfy a child; `interfaces/foo/interface/` has no entry, + # only its own child does, so `interfaces/foo/` is not owned. + entries = [Entry('/interfaces/foo/interface/v0/', ('@canonical/team',))] + paths = [Path('interfaces/foo/', ('interfaces/foo/interface/',))] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['interfaces/foo/'] + + def test_catch_all_does_not_count(self): entries = [Entry('*', ('@canonical/maintainers',))] - assert check_codeowners.find_unowned_dirs(['apt'], entries) == ['apt'] - - def test_entry_without_owner_does_not_count(self): - entries = [Entry('/apt/', ())] - assert check_codeowners.find_unowned_dirs(['apt'], entries) == ['apt'] + paths = [Path('apt/', ('apt/src/',))] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['apt/'] def test_parent_dir_entry_does_not_own_child(self): # `/interfaces/` (the fallback) must not be treated as owning a specific interface. entries = [Entry('/interfaces/', ('@canonical/maintainers',))] - unowned = check_codeowners.find_unowned_dirs(['interfaces/foo'], entries) - assert unowned == ['interfaces/foo'] - - def test_trailing_slash_directory_argument(self): - entries = [Entry('/apt/', ('@canonical/team',))] - assert check_codeowners.find_unowned_dirs(['/apt/'], entries) == [] + paths = [Path('interfaces/foo/', ('interfaces/foo/ruff.toml',))] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['interfaces/foo/'] + + def test_file_needs_own_entry(self): + entries = [Entry('/other', ('@canonical/team',))] + paths = [Path('README.md', ())] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['README.md'] + + def test_dir_with_no_children_needs_own_entry(self): + entries = [Entry('/other/', ('@canonical/team',))] + paths = [Path('apt/', ())] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['apt/'] diff --git a/CODEOWNERS b/CODEOWNERS index 2a7de4b2d..48b53c09f 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -7,11 +7,20 @@ # infra /.github/ @canonical/charmlibs-maintainers +/.gitignore @canonical/charmlibs-maintainers +/.scripts/ @canonical/charmlibs-maintainers +/.template/ @canonical/charmlibs-maintainers +/.workshop/ @canonical/charmlibs-maintainers +/AGENTS.md @canonical/charmlibs-maintainers +/CONTRIBUTING.md @canonical/charmlibs-maintainers /docs.just @canonical/charmlibs-maintainers +/interface-test-requirements.txt @canonical/charmlibs-maintainers /justfile @canonical/charmlibs-maintainers /LICENSE @canonical/charmlibs-maintainers /pyproject.toml @canonical/charmlibs-maintainers /SECURITY.md @canonical/charmlibs-maintainers +/test-requirements.txt @canonical/charmlibs-maintainers +/uv.lock @canonical/charmlibs-maintainers # docs /.docs/ @canonical/charmlibs-maintainers From 05e7e711f54183138d356be7438448a753f98444 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 29 Jun 2026 18:19:00 +1200 Subject: [PATCH 03/19] ci: filter out wildcard entries eagerly --- .scripts/check_codeowners.py | 32 +++++++++++-------------- .scripts/tests/test_check_codeowners.py | 21 ++++++++-------- 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index 17ad38339..f3d0aff8b 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -86,13 +86,19 @@ class Path(typing.NamedTuple): def parse_codeowners(text: str) -> list[Entry]: - """Parse CODEOWNERS file contents into a list of `Entry`s, ignoring comments and blanks.""" + """Parse CODEOWNERS file into `Entry`s, ignoring comments, blanks, and wildcard patterns. + + Wildcard patterns (containing any of `*?[]`, such as the catch-all `*`) are dropped: they + aren't anchored paths, so this check neither validates nor counts them as ownership. + """ entries: list[Entry] = [] for line in text.splitlines(): line = line.split('#', 1)[0].strip() if not line: continue pattern, *owners = line.split() + if any(char in pattern for char in '*?[]'): + continue entries.append(Entry(pattern=pattern, owners=tuple(owners))) return entries @@ -149,7 +155,7 @@ def find_unowned_dirs(entries: Iterable[Entry], paths: Iterable[Path]) -> list[s recursive, so a grandchild entry never satisfies a child, and the catch-all `*` entry isn't a path entry, so it never stands in for an explicit one. """ - entry_targets = {target for entry in entries if (target := entry_target(entry.pattern))} + entry_targets = {entry_target(entry.pattern) for entry in entries} def has_entry(path: str) -> bool: return pathlib.PurePosixPath(path) in entry_targets @@ -165,27 +171,17 @@ def has_entry(path: str) -> bool: def find_orphan_entries(entries: Iterable[Entry], root: pathlib.Path) -> list[str]: - """Return the patterns of path-based entries that don't point at an existing path.""" - orphans: list[str] = [] - for entry in entries: - target = entry_target(entry.pattern) - if target is None: - continue - if not (root / target).exists(): - orphans.append(entry.pattern) - return orphans + """Return the patterns of entries that don't point at an existing path.""" + return [e.pattern for e in entries if not (root / entry_target(e.pattern)).exists()] -def entry_target(pattern: str) -> pathlib.PurePosixPath | None: +def entry_target(pattern: str) -> pathlib.PurePosixPath: """Return the repo-relative path that an anchored CODEOWNERS path pattern points to. - Returns `None` for non-path patterns (those containing glob wildcards, or the - catch-all `*`), which aren't validated as real paths. + Patterns are assumed to be wildcard-free (the parser drops wildcard patterns). CODEOWNERS + paths are anchored to the repo root with a leading slash, and a trailing slash just means + "this is a directory"; neither affects the path. """ - if any(char in pattern for char in '*?[]'): - return None - # CODEOWNERS paths are anchored to the repo root with a leading slash, and a - # trailing slash just means "this is a directory"; neither affects the path. return pathlib.PurePosixPath(pattern.strip('/')) diff --git a/.scripts/tests/test_check_codeowners.py b/.scripts/tests/test_check_codeowners.py index 45f7174fc..7cf9f39cf 100644 --- a/.scripts/tests/test_check_codeowners.py +++ b/.scripts/tests/test_check_codeowners.py @@ -45,6 +45,13 @@ def test_entry_without_owner(self): Entry(pattern='/interfaces/index.json', owners=()) ] + def test_drops_wildcard_patterns(self): + text = '* @canonical/team\n/*.md @canonical/team\n/foo/[abc] @t\n/apt/ @canonical/team\n' + # Only the anchored, wildcard-free entry survives. + assert check_codeowners.parse_codeowners(text) == [ + Entry(pattern='/apt/', owners=('@canonical/team',)) + ] + def test_empty(self): assert check_codeowners.parse_codeowners('') == [] @@ -62,10 +69,6 @@ class TestEntryTarget: def test_path_patterns(self, pattern: str, expected: str): assert check_codeowners.entry_target(pattern) == pathlib.PurePosixPath(expected) - @pytest.mark.parametrize('pattern', ['*', '/*.md', '/foo/*', '/foo/?ar', '/foo/[abc]']) - def test_glob_patterns_are_not_targets(self, pattern: str): - assert check_codeowners.entry_target(pattern) is None - class TestFindOrphanEntries: def test_existing_paths_are_not_orphans(self, tmp_path: pathlib.Path): @@ -87,10 +90,6 @@ def test_missing_path_is_orphan(self, tmp_path: pathlib.Path): ] assert check_codeowners.find_orphan_entries(entries, tmp_path) == ['/gone/'] - def test_glob_entries_are_ignored(self, tmp_path: pathlib.Path): - entries = [Entry('*', ('@canonical/team',)), Entry('/*.md', ('@canonical/team',))] - assert check_codeowners.find_orphan_entries(entries, tmp_path) == [] - class TestBuildPaths: def test_top_level_and_interface_children(self): @@ -163,10 +162,10 @@ def test_rule_is_not_recursive(self): paths = [Path('interfaces/foo/', ('interfaces/foo/interface/',))] assert check_codeowners.find_unowned_dirs(entries, paths) == ['interfaces/foo/'] - def test_catch_all_does_not_count(self): - entries = [Entry('*', ('@canonical/maintainers',))] + def test_no_entries_means_unowned(self): + # The catch-all `*` is dropped by the parser, so find_unowned_dirs never sees it. paths = [Path('apt/', ('apt/src/',))] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['apt/'] + assert check_codeowners.find_unowned_dirs([], paths) == ['apt/'] def test_parent_dir_entry_does_not_own_child(self): # `/interfaces/` (the fallback) must not be treated as owning a specific interface. From f92aaf3641b9f86fa0935e5e655068408d6cfd57 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 29 Jun 2026 18:32:06 +1200 Subject: [PATCH 04/19] ci: use strings consistently (not paths) --- .scripts/check_codeowners.py | 36 +++++++++++++++++-------- .scripts/tests/test_check_codeowners.py | 23 +++++++++++++--- CODEOWNERS | 8 +++--- 3 files changed, 49 insertions(+), 18 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index f3d0aff8b..4cb883c22 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -158,7 +158,7 @@ def find_unowned_dirs(entries: Iterable[Entry], paths: Iterable[Path]) -> list[s entry_targets = {entry_target(entry.pattern) for entry in entries} def has_entry(path: str) -> bool: - return pathlib.PurePosixPath(path) in entry_targets + return path in entry_targets unowned: list[str] = [] for item in paths: @@ -171,18 +171,32 @@ def has_entry(path: str) -> bool: def find_orphan_entries(entries: Iterable[Entry], root: pathlib.Path) -> list[str]: - """Return the patterns of entries that don't point at an existing path.""" - return [e.pattern for e in entries if not (root / entry_target(e.pattern)).exists()] + """Return the patterns of entries that don't point at an existing path of the right kind. - -def entry_target(pattern: str) -> pathlib.PurePosixPath: - """Return the repo-relative path that an anchored CODEOWNERS path pattern points to. - - Patterns are assumed to be wildcard-free (the parser drops wildcard patterns). CODEOWNERS - paths are anchored to the repo root with a leading slash, and a trailing slash just means - "this is a directory"; neither affects the path. + An entry is an orphan if its target doesn't exist, or if its trailing slash disagrees with + reality: directory entries must end with `/` and file entries must not. This enforces the + convention that every directory entry carries a trailing slash, so entries can be compared + against tracked paths verbatim. + """ + orphans: list[str] = [] + for entry in entries: + target = entry_target(entry.pattern) + path = root / target.rstrip('/') + is_dir_entry = target.endswith('/') + if not path.exists() or path.is_dir() != is_dir_entry: + orphans.append(entry.pattern) + return orphans + + +def entry_target(pattern: str) -> str: + """Return the repo-relative path that an anchored CODEOWNERS pattern points to, verbatim. + + Patterns are assumed to be wildcard-free (the parser drops wildcard patterns). Only the + anchoring leading slash is removed; the trailing slash is preserved, so a directory entry + `/apt/` becomes `apt/` and matches the tracked directory path verbatim, enforcing the + convention that directory entries end with `/`. """ - return pathlib.PurePosixPath(pattern.strip('/')) + return pattern.removeprefix('/') if __name__ == '__main__': diff --git a/.scripts/tests/test_check_codeowners.py b/.scripts/tests/test_check_codeowners.py index 7cf9f39cf..f035e8699 100644 --- a/.scripts/tests/test_check_codeowners.py +++ b/.scripts/tests/test_check_codeowners.py @@ -60,14 +60,15 @@ class TestEntryTarget: @pytest.mark.parametrize( ('pattern', 'expected'), [ - ('/apt/', 'apt'), + # The leading slash is stripped; the trailing slash is preserved verbatim. + ('/apt/', 'apt/'), ('/apt', 'apt'), - ('/interfaces/foo/interface/', 'interfaces/foo/interface'), + ('/interfaces/foo/interface/', 'interfaces/foo/interface/'), ('/interfaces/foo/ruff.toml', 'interfaces/foo/ruff.toml'), ], ) def test_path_patterns(self, pattern: str, expected: str): - assert check_codeowners.entry_target(pattern) == pathlib.PurePosixPath(expected) + assert check_codeowners.entry_target(pattern) == expected class TestFindOrphanEntries: @@ -90,6 +91,16 @@ def test_missing_path_is_orphan(self, tmp_path: pathlib.Path): ] assert check_codeowners.find_orphan_entries(entries, tmp_path) == ['/gone/'] + def test_directory_entry_without_trailing_slash_is_orphan(self, tmp_path: pathlib.Path): + (tmp_path / 'apt').mkdir() + entries = [Entry('/apt', ('@canonical/team',))] # missing the trailing slash + assert check_codeowners.find_orphan_entries(entries, tmp_path) == ['/apt'] + + def test_file_entry_with_trailing_slash_is_orphan(self, tmp_path: pathlib.Path): + (tmp_path / 'README.md').touch() + entries = [Entry('/README.md/', ('@canonical/team',))] # stray trailing slash on a file + assert check_codeowners.find_orphan_entries(entries, tmp_path) == ['/README.md/'] + class TestBuildPaths: def test_top_level_and_interface_children(self): @@ -127,6 +138,12 @@ def test_path_with_own_entry_passes(self): paths = [Path('apt/', ('apt/src/',))] assert check_codeowners.find_unowned_dirs(entries, paths) == [] + def test_directory_entry_must_match_trailing_slash(self): + # A directory entry without a trailing slash doesn't match the slashed path verbatim. + entries = [Entry('/apt', ('@canonical/team',))] + paths = [Path('apt/', ('apt/src/',))] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['apt/'] + def test_all_children_owned_passes(self): # Split interface entries: the dir itself has no entry, but each child does. entries = [ diff --git a/CODEOWNERS b/CODEOWNERS index 48b53c09f..fcb66edb8 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -136,13 +136,13 @@ /interfaces/forward_auth/ruff.toml @canonical/identity # /interfaces/gateway_metadata/ -/interfaces/gateway_metadata @canonical/service-mesh +/interfaces/gateway_metadata/ @canonical/service-mesh # /interfaces/istio_ingress_route/ -/interfaces/istio_ingress_route @canonical/service-mesh +/interfaces/istio_ingress_route/ @canonical/service-mesh # /interfaces/istio_metadata/ -/interfaces/istio_metadata @canonical/service-mesh +/interfaces/istio_metadata/ @canonical/service-mesh # /interfaces/istio_request_auth/ /interfaces/istio_request_auth/ @canonical/service-mesh @@ -286,7 +286,7 @@ /interfaces/sloth/ @canonical/tracing-and-profiling # /interfaces/service_mesh/ -/interfaces/service_mesh @canonical/service-mesh +/interfaces/service_mesh/ @canonical/service-mesh # /interfaces/smtp/ /interfaces/smtp/interface/ @canonical/platform-engineering From 102481bb03c0c4f6d77693c275408898a2864887 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 29 Jun 2026 18:34:58 +1200 Subject: [PATCH 05/19] ci: normalize to use a leading slash --- .scripts/check_codeowners.py | 35 ++++-------- .scripts/tests/test_check_codeowners.py | 75 ++++++++++--------------- 2 files changed, 41 insertions(+), 69 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index 4cb883c22..e6937cd05 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -56,7 +56,7 @@ def main() -> int: paths = build_paths(tracked_files(REPO_ROOT)) problems = 0 for path in find_unowned_dirs(entries, paths): - print(f'No explicit CODEOWNERS entry for: /{path}') + print(f'No explicit CODEOWNERS entry for: {path}') problems += 1 for pattern in find_orphan_entries(entries, REPO_ROOT): print(f'CODEOWNERS entry points at a missing path: {pattern}') @@ -76,9 +76,10 @@ class Entry(typing.NamedTuple): class Path(typing.NamedTuple): """A repo path to check, with its immediate children. - `path` is relative to the repo root; directory paths end with `/`. `child_paths` are the - immediate children (also repo-relative, directories ending with `/`), and is empty for - files and for directories with no tracked children. + `path` is anchored from the repo root, in CODEOWNERS form: a leading `/`, and a trailing `/` + for directories (e.g. `/apt/`, `/README.md`). `child_paths` are the immediate children in the + same form, and is empty for files and for directories with no tracked children. Matching this + form lets us compare CODEOWNERS entries verbatim. """ path: str @@ -116,9 +117,9 @@ def tracked_files(root: pathlib.Path) -> list[str]: def build_paths(files: Iterable[str]) -> list[Path]: """Build the `Path`s to check from a list of repo-relative tracked file paths. - Returns an item for every top-level entry and every immediate child of `interfaces/`. A - directory's `path` ends with `/` and its `child_paths` list its immediate children (also - with a trailing `/` for directories); files have an empty `child_paths`. + Returns an item for every top-level entry and every immediate child of `interfaces/`. Each + `path` is rendered in CODEOWNERS form (anchoring leading `/`, trailing `/` for directories) + so entries can be compared verbatim; files have an empty `child_paths`. """ children: dict[str, set[str]] = {} # parent prefix -> immediate child names is_dir: dict[str, bool] = {} # path component prefix -> whether it has children @@ -132,7 +133,7 @@ def build_paths(files: Iterable[str]) -> list[Path]: def render(prefix: str, name: str) -> str: path = f'{prefix}/{name}' if prefix else name - return f'{path}/' if is_dir[path] else path + return f'/{path}/' if is_dir[path] else f'/{path}' def child_paths(prefix: str, name: str) -> tuple[str, ...]: path = f'{prefix}/{name}' if prefix else name @@ -155,7 +156,7 @@ def find_unowned_dirs(entries: Iterable[Entry], paths: Iterable[Path]) -> list[s recursive, so a grandchild entry never satisfies a child, and the catch-all `*` entry isn't a path entry, so it never stands in for an explicit one. """ - entry_targets = {entry_target(entry.pattern) for entry in entries} + entry_targets = {entry.pattern for entry in entries} def has_entry(path: str) -> bool: return path in entry_targets @@ -180,24 +181,12 @@ def find_orphan_entries(entries: Iterable[Entry], root: pathlib.Path) -> list[st """ orphans: list[str] = [] for entry in entries: - target = entry_target(entry.pattern) - path = root / target.rstrip('/') - is_dir_entry = target.endswith('/') + path = root / entry.pattern.strip('/') + is_dir_entry = entry.pattern.endswith('/') if not path.exists() or path.is_dir() != is_dir_entry: orphans.append(entry.pattern) return orphans -def entry_target(pattern: str) -> str: - """Return the repo-relative path that an anchored CODEOWNERS pattern points to, verbatim. - - Patterns are assumed to be wildcard-free (the parser drops wildcard patterns). Only the - anchoring leading slash is removed; the trailing slash is preserved, so a directory entry - `/apt/` becomes `apt/` and matches the tracked directory path verbatim, enforcing the - convention that directory entries end with `/`. - """ - return pattern.removeprefix('/') - - if __name__ == '__main__': sys.exit(main()) diff --git a/.scripts/tests/test_check_codeowners.py b/.scripts/tests/test_check_codeowners.py index f035e8699..f1ef92a6a 100644 --- a/.scripts/tests/test_check_codeowners.py +++ b/.scripts/tests/test_check_codeowners.py @@ -19,8 +19,6 @@ import importlib import pathlib -import pytest - check_codeowners = importlib.import_module('check_codeowners') Entry = check_codeowners.Entry Path = check_codeowners.Path @@ -56,21 +54,6 @@ def test_empty(self): assert check_codeowners.parse_codeowners('') == [] -class TestEntryTarget: - @pytest.mark.parametrize( - ('pattern', 'expected'), - [ - # The leading slash is stripped; the trailing slash is preserved verbatim. - ('/apt/', 'apt/'), - ('/apt', 'apt'), - ('/interfaces/foo/interface/', 'interfaces/foo/interface/'), - ('/interfaces/foo/ruff.toml', 'interfaces/foo/ruff.toml'), - ], - ) - def test_path_patterns(self, pattern: str, expected: str): - assert check_codeowners.entry_target(pattern) == expected - - class TestFindOrphanEntries: def test_existing_paths_are_not_orphans(self, tmp_path: pathlib.Path): (tmp_path / 'apt').mkdir() @@ -113,20 +96,20 @@ def test_top_level_and_interface_children(self): 'interfaces/foo/interface/v0/schema.py', ] assert check_codeowners.build_paths(files) == [ - Path('README.md', ()), - Path('apt/', ('apt/pyproject.toml', 'apt/src/')), - Path('interfaces/', ('interfaces/foo/', 'interfaces/index.json')), - Path('interfaces/foo/', ('interfaces/foo/interface/', 'interfaces/foo/ruff.toml')), - Path('interfaces/index.json', ()), + Path('/README.md', ()), + Path('/apt/', ('/apt/pyproject.toml', '/apt/src/')), + Path('/interfaces/', ('/interfaces/foo/', '/interfaces/index.json')), + Path('/interfaces/foo/', ('/interfaces/foo/interface/', '/interfaces/foo/ruff.toml')), + Path('/interfaces/index.json', ()), ] def test_directories_end_with_slash(self): [apt] = check_codeowners.build_paths(['apt/x.py']) - assert apt.path == 'apt/' + assert apt.path == '/apt/' def test_files_have_no_children(self): [readme] = check_codeowners.build_paths(['README.md']) - assert readme == Path('README.md', ()) + assert readme == Path('/README.md', ()) def test_empty(self): assert check_codeowners.build_paths([]) == [] @@ -135,14 +118,14 @@ def test_empty(self): class TestFindUnownedDirs: def test_path_with_own_entry_passes(self): entries = [Entry('/apt/', ('@canonical/team',))] - paths = [Path('apt/', ('apt/src/',))] + paths = [Path('/apt/', ('/apt/src/',))] assert check_codeowners.find_unowned_dirs(entries, paths) == [] def test_directory_entry_must_match_trailing_slash(self): # A directory entry without a trailing slash doesn't match the slashed path verbatim. entries = [Entry('/apt', ('@canonical/team',))] - paths = [Path('apt/', ('apt/src/',))] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['apt/'] + paths = [Path('/apt/', ('/apt/src/',))] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['/apt/'] def test_all_children_owned_passes(self): # Split interface entries: the dir itself has no entry, but each child does. @@ -150,52 +133,52 @@ def test_all_children_owned_passes(self): Entry('/interfaces/foo/interface/', ('@canonical/team',)), Entry('/interfaces/foo/ruff.toml', ('@canonical/team',)), ] - children = ('interfaces/foo/interface/', 'interfaces/foo/ruff.toml') - paths = [Path('interfaces/foo/', children)] + children = ('/interfaces/foo/interface/', '/interfaces/foo/ruff.toml') + paths = [Path('/interfaces/foo/', children)] assert check_codeowners.find_unowned_dirs(entries, paths) == [] def test_all_children_disowned_passes(self): # Children with ownerless entries are explicitly disowned, which satisfies the parent. entries = [Entry('/foo/a', ()), Entry('/foo/b', ())] - paths = [Path('foo/', ('foo/a', 'foo/b'))] + paths = [Path('/foo/', ('/foo/a', '/foo/b'))] assert check_codeowners.find_unowned_dirs(entries, paths) == [] def test_some_children_unowned_is_unowned(self): entries = [Entry('/interfaces/foo/interface/', ('@canonical/team',))] - children = ('interfaces/foo/interface/', 'interfaces/foo/ruff.toml') - paths = [Path('interfaces/foo/', children)] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['interfaces/foo/'] + children = ('/interfaces/foo/interface/', '/interfaces/foo/ruff.toml') + paths = [Path('/interfaces/foo/', children)] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['/interfaces/foo/'] def test_disowned_path_passes(self): # An ownerless entry for the path itself counts (e.g. interfaces/index.json). entries = [Entry('/interfaces/index.json', ())] - paths = [Path('interfaces/index.json', ())] + paths = [Path('/interfaces/index.json', ())] assert check_codeowners.find_unowned_dirs(entries, paths) == [] def test_rule_is_not_recursive(self): - # A grandchild entry must NOT satisfy a child; `interfaces/foo/interface/` has no entry, - # only its own child does, so `interfaces/foo/` is not owned. + # A grandchild entry must NOT satisfy a child; `/interfaces/foo/interface/` has no entry, + # only its own child does, so `/interfaces/foo/` is not owned. entries = [Entry('/interfaces/foo/interface/v0/', ('@canonical/team',))] - paths = [Path('interfaces/foo/', ('interfaces/foo/interface/',))] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['interfaces/foo/'] + paths = [Path('/interfaces/foo/', ('/interfaces/foo/interface/',))] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['/interfaces/foo/'] def test_no_entries_means_unowned(self): # The catch-all `*` is dropped by the parser, so find_unowned_dirs never sees it. - paths = [Path('apt/', ('apt/src/',))] - assert check_codeowners.find_unowned_dirs([], paths) == ['apt/'] + paths = [Path('/apt/', ('/apt/src/',))] + assert check_codeowners.find_unowned_dirs([], paths) == ['/apt/'] def test_parent_dir_entry_does_not_own_child(self): # `/interfaces/` (the fallback) must not be treated as owning a specific interface. entries = [Entry('/interfaces/', ('@canonical/maintainers',))] - paths = [Path('interfaces/foo/', ('interfaces/foo/ruff.toml',))] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['interfaces/foo/'] + paths = [Path('/interfaces/foo/', ('/interfaces/foo/ruff.toml',))] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['/interfaces/foo/'] def test_file_needs_own_entry(self): entries = [Entry('/other', ('@canonical/team',))] - paths = [Path('README.md', ())] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['README.md'] + paths = [Path('/README.md', ())] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['/README.md'] def test_dir_with_no_children_needs_own_entry(self): entries = [Entry('/other/', ('@canonical/team',))] - paths = [Path('apt/', ())] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['apt/'] + paths = [Path('/apt/', ())] + assert check_codeowners.find_unowned_dirs(entries, paths) == ['/apt/'] From 578171bc8e419ed25251035caa941693d5c94e1a Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 29 Jun 2026 18:37:29 +1200 Subject: [PATCH 06/19] ci: cleanup find_unowned_dirs --- .scripts/check_codeowners.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index e6937cd05..ac75103d8 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -156,16 +156,12 @@ def find_unowned_dirs(entries: Iterable[Entry], paths: Iterable[Path]) -> list[s recursive, so a grandchild entry never satisfies a child, and the catch-all `*` entry isn't a path entry, so it never stands in for an explicit one. """ - entry_targets = {entry.pattern for entry in entries} - - def has_entry(path: str) -> bool: - return path in entry_targets - + owned = {entry.pattern for entry in entries} unowned: list[str] = [] for item in paths: - if has_entry(item.path): + if item.path in owned: continue - if item.child_paths and all(has_entry(child) for child in item.child_paths): + if item.child_paths and owned.issuperset(item.child_paths): continue unowned.append(item.path) return unowned From 1d1da53a76564ffce78bb2f78f1dc6d20b561a51 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 29 Jun 2026 18:50:20 +1200 Subject: [PATCH 07/19] ci: slightly terser --- .scripts/check_codeowners.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index ac75103d8..61dccfdc0 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -178,8 +178,7 @@ def find_orphan_entries(entries: Iterable[Entry], root: pathlib.Path) -> list[st orphans: list[str] = [] for entry in entries: path = root / entry.pattern.strip('/') - is_dir_entry = entry.pattern.endswith('/') - if not path.exists() or path.is_dir() != is_dir_entry: + if not path.exists() or path.is_dir() != entry.pattern.endswith('/'): orphans.append(entry.pattern) return orphans From bfef2b349772377069ed2864aff42ba3d26f86b7 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 29 Jun 2026 18:54:51 +1200 Subject: [PATCH 08/19] ci: orphan -> bad --- .scripts/check_codeowners.py | 16 ++++++++-------- .scripts/just.py | 2 +- .scripts/tests/test_check_codeowners.py | 18 +++++++++--------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index 61dccfdc0..e4cfba95e 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -29,7 +29,7 @@ allowing a directory to be covered by per-child entries (such as an interface's `interface/` plus `ruff.toml`). 2. Every path-based CODEOWNERS entry corresponds to a real path in the repository, so renaming - or removing a directory can't leave behind an orphan entry. + or removing a directory can't leave behind a bad entry. Exit with success (0) if both checks pass, otherwise print the problems to stdout and exit with failure (the number of problems found). @@ -58,11 +58,11 @@ def main() -> int: for path in find_unowned_dirs(entries, paths): print(f'No explicit CODEOWNERS entry for: {path}') problems += 1 - for pattern in find_orphan_entries(entries, REPO_ROOT): + for pattern in find_bad_entries(entries, REPO_ROOT): print(f'CODEOWNERS entry points at a missing path: {pattern}') problems += 1 if problems == 0: - print('Every path has a CODEOWNERS owner, and no entries are orphaned.') + print('Every path has a CODEOWNERS owner, and no entries are bad.') return problems @@ -167,20 +167,20 @@ def find_unowned_dirs(entries: Iterable[Entry], paths: Iterable[Path]) -> list[s return unowned -def find_orphan_entries(entries: Iterable[Entry], root: pathlib.Path) -> list[str]: +def find_bad_entries(entries: Iterable[Entry], root: pathlib.Path) -> list[str]: """Return the patterns of entries that don't point at an existing path of the right kind. - An entry is an orphan if its target doesn't exist, or if its trailing slash disagrees with + An entry is bad if its target doesn't exist, or if its trailing slash disagrees with reality: directory entries must end with `/` and file entries must not. This enforces the convention that every directory entry carries a trailing slash, so entries can be compared against tracked paths verbatim. """ - orphans: list[str] = [] + bad: list[str] = [] for entry in entries: path = root / entry.pattern.strip('/') if not path.exists() or path.is_dir() != entry.pattern.endswith('/'): - orphans.append(entry.pattern) - return orphans + bad.append(entry.pattern) + return bad if __name__ == '__main__': diff --git a/.scripts/just.py b/.scripts/just.py index 6bac603c7..dd4b05148 100755 --- a/.scripts/just.py +++ b/.scripts/just.py @@ -466,7 +466,7 @@ def interfaces_json(argv: list[str]) -> int: @_register def check_codeowners(argv: list[str]) -> int: - """Check every package and interface has a CODEOWNERS entry, and no entries are orphaned.""" + """Check every package and interface has a CODEOWNERS entry, and no entries are bad.""" _parser(check_codeowners).parse_args(argv) # supports `-h` return _run(['.scripts/check_codeowners.py'], check=False) diff --git a/.scripts/tests/test_check_codeowners.py b/.scripts/tests/test_check_codeowners.py index f1ef92a6a..1928981b5 100644 --- a/.scripts/tests/test_check_codeowners.py +++ b/.scripts/tests/test_check_codeowners.py @@ -54,8 +54,8 @@ def test_empty(self): assert check_codeowners.parse_codeowners('') == [] -class TestFindOrphanEntries: - def test_existing_paths_are_not_orphans(self, tmp_path: pathlib.Path): +class TestFindBadEntries: + def test_existing_paths_are_not_bad(self, tmp_path: pathlib.Path): (tmp_path / 'apt').mkdir() (tmp_path / 'interfaces' / 'foo').mkdir(parents=True) (tmp_path / 'interfaces' / 'foo' / 'ruff.toml').touch() @@ -64,25 +64,25 @@ def test_existing_paths_are_not_orphans(self, tmp_path: pathlib.Path): Entry('/interfaces/foo/', ('@canonical/team',)), Entry('/interfaces/foo/ruff.toml', ('@canonical/team',)), ] - assert check_codeowners.find_orphan_entries(entries, tmp_path) == [] + assert check_codeowners.find_bad_entries(entries, tmp_path) == [] - def test_missing_path_is_orphan(self, tmp_path: pathlib.Path): + def test_missing_path_is_bad(self, tmp_path: pathlib.Path): (tmp_path / 'apt').mkdir() entries = [ Entry('/apt/', ('@canonical/team',)), Entry('/gone/', ('@canonical/team',)), ] - assert check_codeowners.find_orphan_entries(entries, tmp_path) == ['/gone/'] + assert check_codeowners.find_bad_entries(entries, tmp_path) == ['/gone/'] - def test_directory_entry_without_trailing_slash_is_orphan(self, tmp_path: pathlib.Path): + def test_directory_entry_without_trailing_slash_is_bad(self, tmp_path: pathlib.Path): (tmp_path / 'apt').mkdir() entries = [Entry('/apt', ('@canonical/team',))] # missing the trailing slash - assert check_codeowners.find_orphan_entries(entries, tmp_path) == ['/apt'] + assert check_codeowners.find_bad_entries(entries, tmp_path) == ['/apt'] - def test_file_entry_with_trailing_slash_is_orphan(self, tmp_path: pathlib.Path): + def test_file_entry_with_trailing_slash_is_bad(self, tmp_path: pathlib.Path): (tmp_path / 'README.md').touch() entries = [Entry('/README.md/', ('@canonical/team',))] # stray trailing slash on a file - assert check_codeowners.find_orphan_entries(entries, tmp_path) == ['/README.md/'] + assert check_codeowners.find_bad_entries(entries, tmp_path) == ['/README.md/'] class TestBuildPaths: From 5a4af1f2a7c603ae0c7ae26b585dfaf8ba37d09f Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 15:54:46 +1200 Subject: [PATCH 09/19] refactor: pathlib based --- .scripts/check_codeowners.py | 147 +++++++--------- .scripts/tests/test_check_codeowners.py | 214 ++++++++++++------------ 2 files changed, 169 insertions(+), 192 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index e4cfba95e..0da863fb0 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -42,24 +42,24 @@ import sys import typing -if typing.TYPE_CHECKING: - from collections.abc import Iterable - # `.scripts/check_codeowners.py` -> repo root is two parents up. REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent CODEOWNERS = REPO_ROOT / 'CODEOWNERS' +ROOT = pathlib.PurePosixPath() # the repository root, as a relative path + + def main() -> int: """Run both CODEOWNERS checks, printing any problems and returning the problem count.""" entries = parse_codeowners(CODEOWNERS.read_text()) - paths = build_paths(tracked_files(REPO_ROOT)) + files = [pathlib.PurePosixPath(f) for f in tracked_files(REPO_ROOT)] problems = 0 - for path in find_unowned_dirs(entries, paths): - print(f'No explicit CODEOWNERS entry for: {path}') + for path in find_unowned_dirs(entries, files): + print(f'No explicit CODEOWNERS entry for: {render(path, files)}') problems += 1 - for pattern in find_bad_entries(entries, REPO_ROOT): - print(f'CODEOWNERS entry points at a missing path: {pattern}') + for entry in find_bad_entries(entries, REPO_ROOT): + print(f'CODEOWNERS entry points at a missing path: {entry.pattern}') problems += 1 if problems == 0: print('Every path has a CODEOWNERS owner, and no entries are bad.') @@ -67,32 +67,20 @@ def main() -> int: class Entry(typing.NamedTuple): - """A single CODEOWNERS entry: a path pattern and its owners.""" + """A single CODEOWNERS entry: the raw `pattern` and its `owners`.""" pattern: str owners: tuple[str, ...] -class Path(typing.NamedTuple): - """A repo path to check, with its immediate children. - - `path` is anchored from the repo root, in CODEOWNERS form: a leading `/`, and a trailing `/` - for directories (e.g. `/apt/`, `/README.md`). `child_paths` are the immediate children in the - same form, and is empty for files and for directories with no tracked children. Matching this - form lets us compare CODEOWNERS entries verbatim. - """ - - path: str - child_paths: tuple[str, ...] - - -def parse_codeowners(text: str) -> list[Entry]: - """Parse CODEOWNERS file into `Entry`s, ignoring comments, blanks, and wildcard patterns. +def parse_codeowners(text: str) -> dict[pathlib.PurePosixPath, Entry]: + """Parse CODEOWNERS into a `{target: entry}` dict, keyed by the repo-relative path it covers. - Wildcard patterns (containing any of `*?[]`, such as the catch-all `*`) are dropped: they - aren't anchored paths, so this check neither validates nor counts them as ownership. + Comments, blanks, and wildcard patterns (containing any of `*?[]`, such as the catch-all `*`) + are ignored: wildcards aren't anchored paths, so this check neither validates nor counts them. + Keys are `PurePosixPath`s, so a trailing slash on a directory entry doesn't affect lookups. """ - entries: list[Entry] = [] + entries: dict[pathlib.PurePosixPath, Entry] = {} for line in text.splitlines(): line = line.split('#', 1)[0].strip() if not line: @@ -100,7 +88,8 @@ def parse_codeowners(text: str) -> list[Entry]: pattern, *owners = line.split() if any(char in pattern for char in '*?[]'): continue - entries.append(Entry(pattern=pattern, owners=tuple(owners))) + target = pathlib.PurePosixPath(pattern.strip('/')) # repo-relative; ignores leading slash + entries[target] = Entry(pattern=pattern, owners=tuple(owners)) return entries @@ -114,72 +103,62 @@ def tracked_files(root: pathlib.Path) -> list[str]: return [file for file in output.split('\0') if file] -def build_paths(files: Iterable[str]) -> list[Path]: - """Build the `Path`s to check from a list of repo-relative tracked file paths. +def find_unowned_dirs( + entries: dict[pathlib.PurePosixPath, Entry], files: list[pathlib.PurePosixPath] +) -> list[pathlib.PurePosixPath]: + """Return the paths to check that have no CODEOWNERS entry, directly or via all their children. - Returns an item for every top-level entry and every immediate child of `interfaces/`. Each - `path` is rendered in CODEOWNERS form (anchoring leading `/`, trailing `/` for directories) - so entries can be compared verbatim; files have an empty `child_paths`. + The paths to check are every top-level path and every immediate child of `interfaces/`. A + path passes if it has its own entry, or (for a directory) every one of its immediate children + has its own entry. An entry without an owner still counts: such a path is explicitly disowned + (like `interfaces/index.json`), which is a deliberate decision. This check is not recursive, + so a grandchild entry never satisfies a child, and the catch-all `*` entry was dropped by the + parser, so it never stands in for an explicit one. """ - children: dict[str, set[str]] = {} # parent prefix -> immediate child names - is_dir: dict[str, bool] = {} # path component prefix -> whether it has children - for file in files: - parts = file.split('/') - for depth in range(len(parts)): - parent = '/'.join(parts[:depth]) - name = parts[depth] - children.setdefault(parent, set()).add(name) - is_dir[f'{parent}/{name}' if parent else name] = depth < len(parts) - 1 - - def render(prefix: str, name: str) -> str: - path = f'{prefix}/{name}' if prefix else name - return f'/{path}/' if is_dir[path] else f'/{path}' - - def child_paths(prefix: str, name: str) -> tuple[str, ...]: - path = f'{prefix}/{name}' if prefix else name - return tuple(sorted(render(path, child) for child in children.get(path, ()))) - - paths = [Path(render('', name), child_paths('', name)) for name in children.get('', ())] - paths += [ - Path(render('interfaces', name), child_paths('interfaces', name)) - for name in children.get('interfaces', ()) - ] - return sorted(paths) - - -def find_unowned_dirs(entries: Iterable[Entry], paths: Iterable[Path]) -> list[str]: - """Return the paths without a CODEOWNERS entry, directly or via all their children. - - A path passes if it has its own entry, or (for a directory) every one of its immediate - children has its own entry. An entry without an owner counts: such a path is explicitly - disowned (like `interfaces/index.json`), which is a deliberate decision. This check is not - recursive, so a grandchild entry never satisfies a child, and the catch-all `*` entry isn't a - path entry, so it never stands in for an explicit one. + targets = [*children_of(ROOT, files), *children_of(pathlib.PurePosixPath('interfaces'), files)] + unowned: list[pathlib.PurePosixPath] = [] + for path in targets: + children = children_of(path, files) + if path not in entries and not (children and entries.keys() >= set(children)): + unowned.append(path) + return sorted(unowned) + + +def children_of( + directory: pathlib.PurePosixPath, files: list[pathlib.PurePosixPath] +) -> list[pathlib.PurePosixPath]: + """Return the immediate children (files and subdirectories) of `directory` tracked in `files`. + + `directory` may be `ROOT` (the repository root). A child is the path one level below + `directory` on the way to a tracked file. """ - owned = {entry.pattern for entry in entries} - unowned: list[str] = [] - for item in paths: - if item.path in owned: - continue - if item.child_paths and owned.issuperset(item.child_paths): - continue - unowned.append(item.path) - return unowned + children = { + directory / file.relative_to(directory).parts[0] + for file in files + if directory == ROOT or directory in file.parents + } + return sorted(children) + + +def render(path: pathlib.PurePosixPath, files: list[pathlib.PurePosixPath]) -> str: + """Render `path` in CODEOWNERS form: a leading `/`, plus a trailing `/` if it's a directory.""" + return f'/{path}/' if children_of(path, files) else f'/{path}' -def find_bad_entries(entries: Iterable[Entry], root: pathlib.Path) -> list[str]: - """Return the patterns of entries that don't point at an existing path of the right kind. +def find_bad_entries( + entries: dict[pathlib.PurePosixPath, Entry], root: pathlib.Path +) -> list[Entry]: + """Return the entries that don't point at an existing path of the right kind. An entry is bad if its target doesn't exist, or if its trailing slash disagrees with reality: directory entries must end with `/` and file entries must not. This enforces the - convention that every directory entry carries a trailing slash, so entries can be compared - against tracked paths verbatim. + convention that every directory entry carries a trailing slash. """ - bad: list[str] = [] - for entry in entries: - path = root / entry.pattern.strip('/') + bad: list[Entry] = [] + for target, entry in entries.items(): + path = root / target if not path.exists() or path.is_dir() != entry.pattern.endswith('/'): - bad.append(entry.pattern) + bad.append(entry) return bad diff --git a/.scripts/tests/test_check_codeowners.py b/.scripts/tests/test_check_codeowners.py index 1928981b5..908547d47 100644 --- a/.scripts/tests/test_check_codeowners.py +++ b/.scripts/tests/test_check_codeowners.py @@ -21,37 +21,78 @@ check_codeowners = importlib.import_module('check_codeowners') Entry = check_codeowners.Entry -Path = check_codeowners.Path +PurePath = pathlib.PurePosixPath + + +def _files(*paths: str) -> list[pathlib.PurePosixPath]: + return [PurePath(p) for p in paths] + + +def _entries(*patterns: str) -> dict[pathlib.PurePosixPath, Entry]: + """Build an entries dict (keyed by repo-relative target) from `pattern` strings.""" + return { + PurePath(p.strip('/')): Entry(pattern=p, owners=('@canonical/team',)) for p in patterns + } class TestParseCodeowners: + def test_keys_by_repo_relative_target(self): + text = '/apt/ @canonical/team # trailing comment\n' + assert check_codeowners.parse_codeowners(text) == { + PurePath('apt'): Entry(pattern='/apt/', owners=('@canonical/team',)) + } + def test_ignores_comments_and_blanks(self): - text = '# a comment\n\n/apt/ @canonical/team # trailing comment\n' - assert check_codeowners.parse_codeowners(text) == [ - Entry(pattern='/apt/', owners=('@canonical/team',)) - ] + text = '# a comment\n\n/apt/ @canonical/team\n' + assert list(check_codeowners.parse_codeowners(text)) == [PurePath('apt')] def test_multiple_owners(self): text = '/foo/ @canonical/one @canonical/two\n' - assert check_codeowners.parse_codeowners(text) == [ - Entry(pattern='/foo/', owners=('@canonical/one', '@canonical/two')) - ] + assert check_codeowners.parse_codeowners(text)[PurePath('foo')] == Entry( + pattern='/foo/', owners=('@canonical/one', '@canonical/two') + ) def test_entry_without_owner(self): text = '/interfaces/index.json\n' - assert check_codeowners.parse_codeowners(text) == [ - Entry(pattern='/interfaces/index.json', owners=()) - ] + assert check_codeowners.parse_codeowners(text)[PurePath('interfaces/index.json')] == Entry( + pattern='/interfaces/index.json', owners=() + ) def test_drops_wildcard_patterns(self): text = '* @canonical/team\n/*.md @canonical/team\n/foo/[abc] @t\n/apt/ @canonical/team\n' # Only the anchored, wildcard-free entry survives. - assert check_codeowners.parse_codeowners(text) == [ - Entry(pattern='/apt/', owners=('@canonical/team',)) - ] + assert list(check_codeowners.parse_codeowners(text)) == [PurePath('apt')] def test_empty(self): - assert check_codeowners.parse_codeowners('') == [] + assert check_codeowners.parse_codeowners('') == {} + + +class TestChildrenOf: + def test_root_children(self): + files = _files('README.md', 'apt/pyproject.toml', 'apt/src/__init__.py') + assert check_codeowners.children_of(check_codeowners.ROOT, files) == _files( + 'README.md', 'apt' + ) + + def test_directory_children(self): + files = _files('apt/pyproject.toml', 'apt/src/__init__.py') + assert check_codeowners.children_of(PurePath('apt'), files) == _files( + 'apt/pyproject.toml', 'apt/src' + ) + + def test_no_children_for_unrelated_dir(self): + files = _files('apt/x.py') + assert check_codeowners.children_of(PurePath('snap'), files) == [] + + +class TestRender: + def test_directory_gets_trailing_slash(self): + files = _files('apt/x.py') + assert check_codeowners.render(PurePath('apt'), files) == '/apt/' + + def test_file_has_no_trailing_slash(self): + files = _files('README.md') + assert check_codeowners.render(PurePath('README.md'), files) == '/README.md' class TestFindBadEntries: @@ -59,126 +100,83 @@ def test_existing_paths_are_not_bad(self, tmp_path: pathlib.Path): (tmp_path / 'apt').mkdir() (tmp_path / 'interfaces' / 'foo').mkdir(parents=True) (tmp_path / 'interfaces' / 'foo' / 'ruff.toml').touch() - entries = [ - Entry('/apt/', ('@canonical/team',)), - Entry('/interfaces/foo/', ('@canonical/team',)), - Entry('/interfaces/foo/ruff.toml', ('@canonical/team',)), - ] + entries = _entries('/apt/', '/interfaces/foo/', '/interfaces/foo/ruff.toml') assert check_codeowners.find_bad_entries(entries, tmp_path) == [] def test_missing_path_is_bad(self, tmp_path: pathlib.Path): (tmp_path / 'apt').mkdir() - entries = [ - Entry('/apt/', ('@canonical/team',)), - Entry('/gone/', ('@canonical/team',)), - ] - assert check_codeowners.find_bad_entries(entries, tmp_path) == ['/gone/'] + entries = _entries('/apt/', '/gone/') + bad = check_codeowners.find_bad_entries(entries, tmp_path) + assert [e.pattern for e in bad] == ['/gone/'] def test_directory_entry_without_trailing_slash_is_bad(self, tmp_path: pathlib.Path): (tmp_path / 'apt').mkdir() - entries = [Entry('/apt', ('@canonical/team',))] # missing the trailing slash - assert check_codeowners.find_bad_entries(entries, tmp_path) == ['/apt'] + entries = _entries('/apt') # missing the trailing slash + bad = check_codeowners.find_bad_entries(entries, tmp_path) + assert [e.pattern for e in bad] == ['/apt'] def test_file_entry_with_trailing_slash_is_bad(self, tmp_path: pathlib.Path): (tmp_path / 'README.md').touch() - entries = [Entry('/README.md/', ('@canonical/team',))] # stray trailing slash on a file - assert check_codeowners.find_bad_entries(entries, tmp_path) == ['/README.md/'] - - -class TestBuildPaths: - def test_top_level_and_interface_children(self): - files = [ - 'README.md', - 'apt/pyproject.toml', - 'apt/src/__init__.py', - 'interfaces/index.json', - 'interfaces/foo/ruff.toml', - 'interfaces/foo/interface/v0/schema.py', - ] - assert check_codeowners.build_paths(files) == [ - Path('/README.md', ()), - Path('/apt/', ('/apt/pyproject.toml', '/apt/src/')), - Path('/interfaces/', ('/interfaces/foo/', '/interfaces/index.json')), - Path('/interfaces/foo/', ('/interfaces/foo/interface/', '/interfaces/foo/ruff.toml')), - Path('/interfaces/index.json', ()), - ] - - def test_directories_end_with_slash(self): - [apt] = check_codeowners.build_paths(['apt/x.py']) - assert apt.path == '/apt/' - - def test_files_have_no_children(self): - [readme] = check_codeowners.build_paths(['README.md']) - assert readme == Path('/README.md', ()) - - def test_empty(self): - assert check_codeowners.build_paths([]) == [] + entries = _entries('/README.md/') # stray trailing slash on a file + bad = check_codeowners.find_bad_entries(entries, tmp_path) + assert [e.pattern for e in bad] == ['/README.md/'] class TestFindUnownedDirs: def test_path_with_own_entry_passes(self): - entries = [Entry('/apt/', ('@canonical/team',))] - paths = [Path('/apt/', ('/apt/src/',))] - assert check_codeowners.find_unowned_dirs(entries, paths) == [] + entries = _entries('/apt/') + files = _files('apt/src/x.py') + assert check_codeowners.find_unowned_dirs(entries, files) == [] - def test_directory_entry_must_match_trailing_slash(self): - # A directory entry without a trailing slash doesn't match the slashed path verbatim. - entries = [Entry('/apt', ('@canonical/team',))] - paths = [Path('/apt/', ('/apt/src/',))] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['/apt/'] + def test_trailing_slash_is_ignored_when_matching(self): + # Matching is slash-insensitive (PurePosixPath); find_bad_entries enforces the slash. + entries = _entries('/apt') # no trailing slash, but still matches the apt directory + files = _files('apt/src/x.py') + assert check_codeowners.find_unowned_dirs(entries, files) == [] def test_all_children_owned_passes(self): - # Split interface entries: the dir itself has no entry, but each child does. - entries = [ - Entry('/interfaces/foo/interface/', ('@canonical/team',)), - Entry('/interfaces/foo/ruff.toml', ('@canonical/team',)), - ] - children = ('/interfaces/foo/interface/', '/interfaces/foo/ruff.toml') - paths = [Path('/interfaces/foo/', children)] - assert check_codeowners.find_unowned_dirs(entries, paths) == [] + # Split interface entries: interfaces/foo has no entry, but each of its children does. + entries = _entries( + '/interfaces/', '/interfaces/foo/interface/', '/interfaces/foo/ruff.toml' + ) + files = _files('interfaces/foo/interface/v0/schema.py', 'interfaces/foo/ruff.toml') + assert check_codeowners.find_unowned_dirs(entries, files) == [] def test_all_children_disowned_passes(self): # Children with ownerless entries are explicitly disowned, which satisfies the parent. - entries = [Entry('/foo/a', ()), Entry('/foo/b', ())] - paths = [Path('/foo/', ('/foo/a', '/foo/b'))] - assert check_codeowners.find_unowned_dirs(entries, paths) == [] + entries = { + PurePath('interfaces'): Entry('/interfaces/', ('@canonical/team',)), + PurePath('interfaces/foo/interface'): Entry('/interfaces/foo/interface/', ()), + PurePath('interfaces/foo/ruff.toml'): Entry('/interfaces/foo/ruff.toml', ()), + } + files = _files('interfaces/foo/interface/v0/schema.py', 'interfaces/foo/ruff.toml') + assert check_codeowners.find_unowned_dirs(entries, files) == [] def test_some_children_unowned_is_unowned(self): - entries = [Entry('/interfaces/foo/interface/', ('@canonical/team',))] - children = ('/interfaces/foo/interface/', '/interfaces/foo/ruff.toml') - paths = [Path('/interfaces/foo/', children)] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['/interfaces/foo/'] - - def test_disowned_path_passes(self): - # An ownerless entry for the path itself counts (e.g. interfaces/index.json). - entries = [Entry('/interfaces/index.json', ())] - paths = [Path('/interfaces/index.json', ())] - assert check_codeowners.find_unowned_dirs(entries, paths) == [] + # /interfaces/ is owned; only interfaces/foo (whose ruff.toml lacks an entry) is unowned. + entries = _entries('/interfaces/', '/interfaces/foo/interface/') + files = _files('interfaces/foo/interface/v0/schema.py', 'interfaces/foo/ruff.toml') + assert check_codeowners.find_unowned_dirs(entries, files) == [PurePath('interfaces/foo')] def test_rule_is_not_recursive(self): - # A grandchild entry must NOT satisfy a child; `/interfaces/foo/interface/` has no entry, - # only its own child does, so `/interfaces/foo/` is not owned. - entries = [Entry('/interfaces/foo/interface/v0/', ('@canonical/team',))] - paths = [Path('/interfaces/foo/', ('/interfaces/foo/interface/',))] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['/interfaces/foo/'] + # A grandchild entry must NOT satisfy a child; `interfaces/foo/interface` has no entry, + # only its own child does, so `interfaces/foo` is not owned. + entries = _entries('/interfaces/', '/interfaces/foo/interface/v0/') + files = _files('interfaces/foo/interface/v0/schema.py') + assert check_codeowners.find_unowned_dirs(entries, files) == [PurePath('interfaces/foo')] def test_no_entries_means_unowned(self): # The catch-all `*` is dropped by the parser, so find_unowned_dirs never sees it. - paths = [Path('/apt/', ('/apt/src/',))] - assert check_codeowners.find_unowned_dirs([], paths) == ['/apt/'] + files = _files('apt/src/x.py') + assert check_codeowners.find_unowned_dirs({}, files) == [PurePath('apt')] def test_parent_dir_entry_does_not_own_child(self): # `/interfaces/` (the fallback) must not be treated as owning a specific interface. - entries = [Entry('/interfaces/', ('@canonical/maintainers',))] - paths = [Path('/interfaces/foo/', ('/interfaces/foo/ruff.toml',))] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['/interfaces/foo/'] - - def test_file_needs_own_entry(self): - entries = [Entry('/other', ('@canonical/team',))] - paths = [Path('/README.md', ())] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['/README.md'] - - def test_dir_with_no_children_needs_own_entry(self): - entries = [Entry('/other/', ('@canonical/team',))] - paths = [Path('/apt/', ())] - assert check_codeowners.find_unowned_dirs(entries, paths) == ['/apt/'] + entries = _entries('/interfaces/') + files = _files('interfaces/foo/ruff.toml') + assert check_codeowners.find_unowned_dirs(entries, files) == [PurePath('interfaces/foo')] + + def test_top_level_file_needs_own_entry(self): + entries = _entries('/other') + files = _files('README.md') + assert check_codeowners.find_unowned_dirs(entries, files) == [PurePath('README.md')] From cfeeeb7436d2e8e37afb37c593902228616e51c3 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 18:13:48 +1200 Subject: [PATCH 10/19] ci: manually rewrite codeowners script --- .scripts/check_codeowners.py | 154 +++++++++++++---------------------- 1 file changed, 56 insertions(+), 98 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index 0da863fb0..0a0082fee 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -40,127 +40,85 @@ import pathlib import subprocess import sys -import typing # `.scripts/check_codeowners.py` -> repo root is two parents up. REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent -CODEOWNERS = REPO_ROOT / 'CODEOWNERS' -ROOT = pathlib.PurePosixPath() # the repository root, as a relative path +class TrackedFiles: + def __init__(self): + self._files = self._get_tracked_files() + self._lookup: set[pathlib.Path] = set() + self._highest_lookup_depth = 0 + + def _get_tracked_files(self) -> list[pathlib.Path]: + output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=REPO_ROOT, text=True) + return [pathlib.Path(file) for file in output.split('\0') if file] + + def children(self, path: pathlib.Path) -> list[pathlib.Path]: + """Return the immediate children of `path` that are tracked files or directories.""" + for i in range(self._highest_lookup_depth + 1, len(path.parts) + 2): + self._lookup.update(pathlib.Path(*p.parts[:i]) for p in self._files) + self._highest_lookup_depth = i + return sorted( + rel_path + for p in path.iterdir() + if (rel_path := p.relative_to(REPO_ROOT)) in self._lookup + ) def main() -> int: """Run both CODEOWNERS checks, printing any problems and returning the problem count.""" - entries = parse_codeowners(CODEOWNERS.read_text()) - files = [pathlib.PurePosixPath(f) for f in tracked_files(REPO_ROOT)] + entries = parse_codeowners((REPO_ROOT / 'CODEOWNERS').read_text()) + tracked = TrackedFiles() + owner_required = [*tracked.children(REPO_ROOT), *tracked.children(REPO_ROOT / 'interfaces')] problems = 0 - for path in find_unowned_dirs(entries, files): - print(f'No explicit CODEOWNERS entry for: {render(path, files)}') - problems += 1 - for entry in find_bad_entries(entries, REPO_ROOT): - print(f'CODEOWNERS entry points at a missing path: {entry.pattern}') - problems += 1 - if problems == 0: - print('Every path has a CODEOWNERS owner, and no entries are bad.') + # Check tracked files against CODEOWNERS entries. + for path in owner_required: + if path not in entries: + if not path.is_dir(): + print(f'No explicit CODEOWNERS for: /{path}') + problems += 1 + elif not all(p in entries for p in tracked.children(REPO_ROOT / path)): + print(f"No explicit CODEOWNERS for: /{path}/ (and its children aren't all owned)") + problems += 1 + # Check CODEOWNERS entries. + for target, pattern in entries.items(): + path = REPO_ROOT / target + if not path.exists(): + print(f'CODEOWNERS entry points at a missing path: {pattern}') + problems += 1 + if path.is_dir() != pattern.endswith('/'): + print(f'CODEOWNERS entry must have a trailing slash iff it is a dir: {pattern}') + problems += 1 + if not pattern.startswith('/'): + print(f'CODEOWNERS entry must be anchored with a leading /: {pattern}') + problems += 1 + if not problems: + print('No problems found in CODEOWNERS :)') return problems -class Entry(typing.NamedTuple): - """A single CODEOWNERS entry: the raw `pattern` and its `owners`.""" - - pattern: str - owners: tuple[str, ...] - - -def parse_codeowners(text: str) -> dict[pathlib.PurePosixPath, Entry]: - """Parse CODEOWNERS into a `{target: entry}` dict, keyed by the repo-relative path it covers. +def parse_codeowners(text: str) -> dict[pathlib.Path, str]: + """Parse CODEOWNERS into a `{target: pattern}` dict, keyed by the repo-relative path it covers. Comments, blanks, and wildcard patterns (containing any of `*?[]`, such as the catch-all `*`) are ignored: wildcards aren't anchored paths, so this check neither validates nor counts them. - Keys are `PurePosixPath`s, so a trailing slash on a directory entry doesn't affect lookups. + Keys are `pathlib.Path`s, so a trailing slash on a directory entry doesn't affect lookups. """ - entries: dict[pathlib.PurePosixPath, Entry] = {} + entries: dict[pathlib.Path, str] = {} for line in text.splitlines(): - line = line.split('#', 1)[0].strip() - if not line: + entry, _, *_ = line.partition('#') # drop trailing comment + entry = entry.strip() + if not entry: continue - pattern, *owners = line.split() + pattern, *_owners = entry.split() if any(char in pattern for char in '*?[]'): continue - target = pathlib.PurePosixPath(pattern.strip('/')) # repo-relative; ignores leading slash - entries[target] = Entry(pattern=pattern, owners=tuple(owners)) + target = pathlib.Path(pattern.strip('/')) # to be interpreted relative to REPO_ROOT + entries[target] = pattern return entries -def tracked_files(root: pathlib.Path) -> list[str]: - """Return the repo-relative POSIX paths of all files tracked by git in `root`. - - Only tracked files are returned, so untracked and ignored paths (e.g. `.venv/`, caches) - don't need a CODEOWNERS entry. - """ - output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=root, text=True) - return [file for file in output.split('\0') if file] - - -def find_unowned_dirs( - entries: dict[pathlib.PurePosixPath, Entry], files: list[pathlib.PurePosixPath] -) -> list[pathlib.PurePosixPath]: - """Return the paths to check that have no CODEOWNERS entry, directly or via all their children. - - The paths to check are every top-level path and every immediate child of `interfaces/`. A - path passes if it has its own entry, or (for a directory) every one of its immediate children - has its own entry. An entry without an owner still counts: such a path is explicitly disowned - (like `interfaces/index.json`), which is a deliberate decision. This check is not recursive, - so a grandchild entry never satisfies a child, and the catch-all `*` entry was dropped by the - parser, so it never stands in for an explicit one. - """ - targets = [*children_of(ROOT, files), *children_of(pathlib.PurePosixPath('interfaces'), files)] - unowned: list[pathlib.PurePosixPath] = [] - for path in targets: - children = children_of(path, files) - if path not in entries and not (children and entries.keys() >= set(children)): - unowned.append(path) - return sorted(unowned) - - -def children_of( - directory: pathlib.PurePosixPath, files: list[pathlib.PurePosixPath] -) -> list[pathlib.PurePosixPath]: - """Return the immediate children (files and subdirectories) of `directory` tracked in `files`. - - `directory` may be `ROOT` (the repository root). A child is the path one level below - `directory` on the way to a tracked file. - """ - children = { - directory / file.relative_to(directory).parts[0] - for file in files - if directory == ROOT or directory in file.parents - } - return sorted(children) - - -def render(path: pathlib.PurePosixPath, files: list[pathlib.PurePosixPath]) -> str: - """Render `path` in CODEOWNERS form: a leading `/`, plus a trailing `/` if it's a directory.""" - return f'/{path}/' if children_of(path, files) else f'/{path}' - - -def find_bad_entries( - entries: dict[pathlib.PurePosixPath, Entry], root: pathlib.Path -) -> list[Entry]: - """Return the entries that don't point at an existing path of the right kind. - - An entry is bad if its target doesn't exist, or if its trailing slash disagrees with - reality: directory entries must end with `/` and file entries must not. This enforces the - convention that every directory entry carries a trailing slash. - """ - bad: list[Entry] = [] - for target, entry in entries.items(): - path = root / target - if not path.exists() or path.is_dir() != entry.pattern.endswith('/'): - bad.append(entry) - return bad - - if __name__ == '__main__': sys.exit(main()) From c0140bcca51448feed0a0d73aa884602fe36aaf0 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 18:22:51 +1200 Subject: [PATCH 11/19] ci: refactor script to facilitate testing --- .scripts/check_codeowners.py | 69 +++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index 0a0082fee..c036fcc2f 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -46,57 +46,39 @@ class TrackedFiles: - def __init__(self): - self._files = self._get_tracked_files() + def __init__(self, root: pathlib.Path): + self._root = root + self._files = self._get_tracked_files(root) self._lookup: set[pathlib.Path] = set() self._highest_lookup_depth = 0 - def _get_tracked_files(self) -> list[pathlib.Path]: - output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=REPO_ROOT, text=True) + def _get_tracked_files(self, root: pathlib.Path) -> list[pathlib.Path]: + output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=root, text=True) return [pathlib.Path(file) for file in output.split('\0') if file] - def children(self, path: pathlib.Path) -> list[pathlib.Path]: + def children(self, path: pathlib.Path | str | None = None) -> list[pathlib.Path]: """Return the immediate children of `path` that are tracked files or directories.""" + path = self._root / path if path is not None else self._root for i in range(self._highest_lookup_depth + 1, len(path.parts) + 2): self._lookup.update(pathlib.Path(*p.parts[:i]) for p in self._files) self._highest_lookup_depth = i return sorted( rel_path for p in path.iterdir() - if (rel_path := p.relative_to(REPO_ROOT)) in self._lookup + if (rel_path := p.relative_to(self._root)) in self._lookup ) def main() -> int: """Run both CODEOWNERS checks, printing any problems and returning the problem count.""" entries = parse_codeowners((REPO_ROOT / 'CODEOWNERS').read_text()) - tracked = TrackedFiles() - owner_required = [*tracked.children(REPO_ROOT), *tracked.children(REPO_ROOT / 'interfaces')] - problems = 0 - # Check tracked files against CODEOWNERS entries. - for path in owner_required: - if path not in entries: - if not path.is_dir(): - print(f'No explicit CODEOWNERS for: /{path}') - problems += 1 - elif not all(p in entries for p in tracked.children(REPO_ROOT / path)): - print(f"No explicit CODEOWNERS for: /{path}/ (and its children aren't all owned)") - problems += 1 - # Check CODEOWNERS entries. - for target, pattern in entries.items(): - path = REPO_ROOT / target - if not path.exists(): - print(f'CODEOWNERS entry points at a missing path: {pattern}') - problems += 1 - if path.is_dir() != pattern.endswith('/'): - print(f'CODEOWNERS entry must have a trailing slash iff it is a dir: {pattern}') - problems += 1 - if not pattern.startswith('/'): - print(f'CODEOWNERS entry must be anchored with a leading /: {pattern}') - problems += 1 - if not problems: - print('No problems found in CODEOWNERS :)') - return problems + tracked = TrackedFiles(REPO_ROOT) + problems = check(entries, tracked) + if problems: + print('\n'.join(problems)) + else: + print('No problems found with CODEOWNERS :)') + return len(problems) def parse_codeowners(text: str) -> dict[pathlib.Path, str]: @@ -120,5 +102,26 @@ def parse_codeowners(text: str) -> dict[pathlib.Path, str]: return entries +def check(entries: dict[pathlib.Path, str], tracked: TrackedFiles) -> list[str]: + problems: list[str] = [] + # Check tracked files against CODEOWNERS entries. + for path in *tracked.children(), *tracked.children('interfaces'): + if path not in entries: + if not path.is_dir(): + problems.append(f'No explicit CODEOWNERS for: /{path}') + elif not all(p in entries for p in tracked.children(path)): + problems.append(f"No explicit CODEOWNERS for: /{path}/ (and its children aren't all owned)") + # Check CODEOWNERS entries. + for target, pattern in entries.items(): + path = REPO_ROOT / target + if not path.exists(): + problems.append(f'CODEOWNERS entry points at a missing path: {pattern}') + if path.is_dir() != pattern.endswith('/'): + problems.append(f'CODEOWNERS entry must have a trailing slash iff it is a dir: {pattern}') + if not pattern.startswith('/'): + problems.append(f'CODEOWNERS entry must be anchored with a leading /: {pattern}') + return problems + + if __name__ == '__main__': sys.exit(main()) From 359693a187de49f2f69293e1da96999af590eb8e Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 18:50:51 +1200 Subject: [PATCH 12/19] ci: fixes from review --- .scripts/check_codeowners.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index c036fcc2f..72c2ea1de 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -59,7 +59,10 @@ def _get_tracked_files(self, root: pathlib.Path) -> list[pathlib.Path]: def children(self, path: pathlib.Path | str | None = None) -> list[pathlib.Path]: """Return the immediate children of `path` that are tracked files or directories.""" path = self._root / path if path is not None else self._root - for i in range(self._highest_lookup_depth + 1, len(path.parts) + 2): + assert path.is_dir() + for i in range( + self._highest_lookup_depth + 1, len(path.relative_to(self._root).parts) + 2 + ): self._lookup.update(pathlib.Path(*p.parts[:i]) for p in self._files) self._highest_lookup_depth = i return sorted( @@ -90,7 +93,7 @@ def parse_codeowners(text: str) -> dict[pathlib.Path, str]: """ entries: dict[pathlib.Path, str] = {} for line in text.splitlines(): - entry, _, *_ = line.partition('#') # drop trailing comment + entry, _, _ = line.partition('#') # drop trailing comments entry = entry.strip() if not entry: continue @@ -116,7 +119,7 @@ def check(entries: dict[pathlib.Path, str], tracked: TrackedFiles) -> list[str]: path = REPO_ROOT / target if not path.exists(): problems.append(f'CODEOWNERS entry points at a missing path: {pattern}') - if path.is_dir() != pattern.endswith('/'): + elif path.is_dir() != pattern.endswith('/'): problems.append(f'CODEOWNERS entry must have a trailing slash iff it is a dir: {pattern}') if not pattern.startswith('/'): problems.append(f'CODEOWNERS entry must be anchored with a leading /: {pattern}') From efd012ac2fdb31b449fe3b8f6480e70a996263f0 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 19:23:09 +1200 Subject: [PATCH 13/19] ci: try capped but greedy building instead --- .scripts/check_codeowners.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index 72c2ea1de..7a9779612 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -46,11 +46,14 @@ class TrackedFiles: - def __init__(self, root: pathlib.Path): + def __init__(self, root: pathlib.Path, max_depth: int = 4): self._root = root self._files = self._get_tracked_files(root) + # Build efficient lookup for children(). self._lookup: set[pathlib.Path] = set() - self._highest_lookup_depth = 0 + self._max_depth = max_depth + for i in range(1, max_depth + 1): + self._lookup.update(pathlib.Path(*p.parts[:i]) for p in self._files) def _get_tracked_files(self, root: pathlib.Path) -> list[pathlib.Path]: output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=root, text=True) @@ -60,11 +63,10 @@ def children(self, path: pathlib.Path | str | None = None) -> list[pathlib.Path] """Return the immediate children of `path` that are tracked files or directories.""" path = self._root / path if path is not None else self._root assert path.is_dir() - for i in range( - self._highest_lookup_depth + 1, len(path.relative_to(self._root).parts) + 2 - ): - self._lookup.update(pathlib.Path(*p.parts[:i]) for p in self._files) - self._highest_lookup_depth = i + if len(path.relative_to(self._root).parts) >= self._max_depth - 1: + raise ValueError( + f'{path.relative_to(self._root)} is too deep, initialise with higher max_depth (now {self._max_depth})' + ) return sorted( rel_path for p in path.iterdir() From 4c7925f8f81ba2fd7a1f21cf87e684768fe3288c Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 19:35:51 +1200 Subject: [PATCH 14/19] refactor: greedily build the children map --- .scripts/check_codeowners.py | 43 +++++++++++------------------------- 1 file changed, 13 insertions(+), 30 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index 7a9779612..b4205c543 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -24,11 +24,8 @@ Two checks are performed against every top-level path and every immediate child of `interfaces/`: 1. Each path is owned, meaning either the path itself has a CODEOWNERS entry with an owner, or - (for a directory) every one of its immediate children is owned. This ensures ownership isn't - left to fall through to the repository maintainers via the catch-all `*` entry, while still - allowing a directory to be covered by per-child entries (such as an interface's `interface/` - plus `ruff.toml`). -2. Every path-based CODEOWNERS entry corresponds to a real path in the repository, so renaming + (for a directory) every one of its immediate children has a direct entry. +2. Every concrete CODEOWNERS entry corresponds to a real path in the repository, so renaming or removing a directory can't leave behind a bad entry. Exit with success (0) if both checks pass, otherwise print the problems to stdout @@ -46,32 +43,20 @@ class TrackedFiles: - def __init__(self, root: pathlib.Path, max_depth: int = 4): - self._root = root - self._files = self._get_tracked_files(root) - # Build efficient lookup for children(). - self._lookup: set[pathlib.Path] = set() - self._max_depth = max_depth - for i in range(1, max_depth + 1): - self._lookup.update(pathlib.Path(*p.parts[:i]) for p in self._files) - - def _get_tracked_files(self, root: pathlib.Path) -> list[pathlib.Path]: + def __init__(self, root: pathlib.Path | str): + self._children: dict[pathlib.Path, set[pathlib.Path]] = {} + for path in self._get_tracked_files(root): + for p in (path, *path.parents[:-1]): # Don't include the root as a child ('.'). + self._children.setdefault(p.parent, set()).add(p) + + @staticmethod + def _get_tracked_files(root: pathlib.Path | str) -> list[pathlib.Path]: output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=root, text=True) return [pathlib.Path(file) for file in output.split('\0') if file] - def children(self, path: pathlib.Path | str | None = None) -> list[pathlib.Path]: + def children(self, path: pathlib.Path | str = '.') -> list[pathlib.Path]: """Return the immediate children of `path` that are tracked files or directories.""" - path = self._root / path if path is not None else self._root - assert path.is_dir() - if len(path.relative_to(self._root).parts) >= self._max_depth - 1: - raise ValueError( - f'{path.relative_to(self._root)} is too deep, initialise with higher max_depth (now {self._max_depth})' - ) - return sorted( - rel_path - for p in path.iterdir() - if (rel_path := p.relative_to(self._root)) in self._lookup - ) + return sorted(self._children[pathlib.Path(path)]) def main() -> int: @@ -89,9 +74,7 @@ def main() -> int: def parse_codeowners(text: str) -> dict[pathlib.Path, str]: """Parse CODEOWNERS into a `{target: pattern}` dict, keyed by the repo-relative path it covers. - Comments, blanks, and wildcard patterns (containing any of `*?[]`, such as the catch-all `*`) - are ignored: wildcards aren't anchored paths, so this check neither validates nor counts them. - Keys are `pathlib.Path`s, so a trailing slash on a directory entry doesn't affect lookups. + Comments, blanks, and wildcard patterns (containing any of `*?[]`) are ignored. """ entries: dict[pathlib.Path, str] = {} for line in text.splitlines(): From fe280193312c72ce575a9d4b282a0e36de88c7d2 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 19:44:19 +1200 Subject: [PATCH 15/19] ci: drop object oriented approach --- .scripts/check_codeowners.py | 40 ++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index b4205c543..a6ee098f9 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -42,28 +42,11 @@ REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent -class TrackedFiles: - def __init__(self, root: pathlib.Path | str): - self._children: dict[pathlib.Path, set[pathlib.Path]] = {} - for path in self._get_tracked_files(root): - for p in (path, *path.parents[:-1]): # Don't include the root as a child ('.'). - self._children.setdefault(p.parent, set()).add(p) - - @staticmethod - def _get_tracked_files(root: pathlib.Path | str) -> list[pathlib.Path]: - output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=root, text=True) - return [pathlib.Path(file) for file in output.split('\0') if file] - - def children(self, path: pathlib.Path | str = '.') -> list[pathlib.Path]: - """Return the immediate children of `path` that are tracked files or directories.""" - return sorted(self._children[pathlib.Path(path)]) - - def main() -> int: """Run both CODEOWNERS checks, printing any problems and returning the problem count.""" entries = parse_codeowners((REPO_ROOT / 'CODEOWNERS').read_text()) - tracked = TrackedFiles(REPO_ROOT) - problems = check(entries, tracked) + children = map_children(get_tracked_files(REPO_ROOT)) + problems = check(entries, children) if problems: print('\n'.join(problems)) else: @@ -90,14 +73,27 @@ def parse_codeowners(text: str) -> dict[pathlib.Path, str]: return entries -def check(entries: dict[pathlib.Path, str], tracked: TrackedFiles) -> list[str]: +def get_tracked_files(root: pathlib.Path | str) -> list[pathlib.Path]: + output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=root, text=True) + return [pathlib.Path(file) for file in output.split('\0') if file] + + +def map_children(files: list[pathlib.Path]) -> dict[pathlib.Path, list[pathlib.Path]]: + children: dict[pathlib.Path, set[pathlib.Path]] = {} + for path in files: + for p in (path, *path.parents[:-1]): # Don't include the root as a child ('.'). + children.setdefault(p.parent, set()).add(p) + return {k: sorted(v) for k, v in children.items()} + + +def check(entries: dict[pathlib.Path, str], children: dict[pathlib.Path, list[pathlib.Path]]) -> list[str]: problems: list[str] = [] # Check tracked files against CODEOWNERS entries. - for path in *tracked.children(), *tracked.children('interfaces'): + for path in *children[pathlib.Path()], *children[pathlib.Path('interfaces')]: if path not in entries: if not path.is_dir(): problems.append(f'No explicit CODEOWNERS for: /{path}') - elif not all(p in entries for p in tracked.children(path)): + elif not all(p in entries for p in children[path]): problems.append(f"No explicit CODEOWNERS for: /{path}/ (and its children aren't all owned)") # Check CODEOWNERS entries. for target, pattern in entries.items(): From 46384ef18dd0ad8622e82bd4362776c3466a667b Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 20:04:43 +1200 Subject: [PATCH 16/19] ci: fix linting, tests --- .scripts/check_codeowners.py | 43 +++-- .scripts/tests/test_check_codeowners.py | 208 ++++++++++-------------- 2 files changed, 118 insertions(+), 133 deletions(-) diff --git a/.scripts/check_codeowners.py b/.scripts/check_codeowners.py index a6ee098f9..9e58b62a9 100755 --- a/.scripts/check_codeowners.py +++ b/.scripts/check_codeowners.py @@ -74,11 +74,17 @@ def parse_codeowners(text: str) -> dict[pathlib.Path, str]: def get_tracked_files(root: pathlib.Path | str) -> list[pathlib.Path]: + """Return the repo-relative paths of all files tracked by git in `root`.""" output = subprocess.check_output(['git', 'ls-files', '-z'], cwd=root, text=True) return [pathlib.Path(file) for file in output.split('\0') if file] def map_children(files: list[pathlib.Path]) -> dict[pathlib.Path, list[pathlib.Path]]: + """Map each tracked directory to its sorted immediate children (files and subdirectories). + + The repository root is keyed as `pathlib.Path()` (i.e. `.`). A path is a directory iff it + appears as a key; the tracked files are the paths that never appear as keys. + """ children: dict[pathlib.Path, set[pathlib.Path]] = {} for path in files: for p in (path, *path.parents[:-1]): # Don't include the root as a child ('.'). @@ -86,22 +92,35 @@ def map_children(files: list[pathlib.Path]) -> dict[pathlib.Path, list[pathlib.P return {k: sorted(v) for k, v in children.items()} -def check(entries: dict[pathlib.Path, str], children: dict[pathlib.Path, list[pathlib.Path]]) -> list[str]: +def check( + entries: dict[pathlib.Path, str], children: dict[pathlib.Path, list[pathlib.Path]] +) -> list[str]: + """Return a list of problems found between the CODEOWNERS `entries` and the tracked tree. + + Everything is checked against the tracked tree (`children`) rather than the filesystem, so + untracked and ignored paths are irrelevant. A path is a tracked directory iff it's a key in + `children`; `all_paths` is every tracked file and directory. + """ problems: list[str] = [] - # Check tracked files against CODEOWNERS entries. + # Check that each top-level path and immediate child of interfaces/ is owned. for path in *children[pathlib.Path()], *children[pathlib.Path('interfaces')]: - if path not in entries: - if not path.is_dir(): - problems.append(f'No explicit CODEOWNERS for: /{path}') - elif not all(p in entries for p in children[path]): - problems.append(f"No explicit CODEOWNERS for: /{path}/ (and its children aren't all owned)") - # Check CODEOWNERS entries. + if path in entries: + continue + if path not in children: # a file: it needs its own entry + problems.append(f'No explicit CODEOWNERS for: /{path}') + elif not all(child in entries for child in children[path]): + problems.append( + f"No explicit CODEOWNERS for: /{path}/ (and its children aren't all owned)" + ) + # Check that each entry points at a tracked path of the matching kind. + all_paths = {child for siblings in children.values() for child in siblings} for target, pattern in entries.items(): - path = REPO_ROOT / target - if not path.exists(): + if target not in all_paths: problems.append(f'CODEOWNERS entry points at a missing path: {pattern}') - elif path.is_dir() != pattern.endswith('/'): - problems.append(f'CODEOWNERS entry must have a trailing slash iff it is a dir: {pattern}') + elif (target in children) != pattern.endswith('/'): + problems.append( + f'CODEOWNERS entry must have a trailing slash iff it is a dir: {pattern}' + ) if not pattern.startswith('/'): problems.append(f'CODEOWNERS entry must be anchored with a leading /: {pattern}') return problems diff --git a/.scripts/tests/test_check_codeowners.py b/.scripts/tests/test_check_codeowners.py index 908547d47..547871a7a 100644 --- a/.scripts/tests/test_check_codeowners.py +++ b/.scripts/tests/test_check_codeowners.py @@ -20,163 +20,129 @@ import pathlib check_codeowners = importlib.import_module('check_codeowners') -Entry = check_codeowners.Entry -PurePath = pathlib.PurePosixPath +Path = pathlib.Path -def _files(*paths: str) -> list[pathlib.PurePosixPath]: - return [PurePath(p) for p in paths] +def _children(*files: str) -> dict[pathlib.Path, list[pathlib.Path]]: + """Build the parent -> children map from repo-relative file path strings.""" + return check_codeowners.map_children([Path(f) for f in files]) -def _entries(*patterns: str) -> dict[pathlib.PurePosixPath, Entry]: - """Build an entries dict (keyed by repo-relative target) from `pattern` strings.""" - return { - PurePath(p.strip('/')): Entry(pattern=p, owners=('@canonical/team',)) for p in patterns - } +def _entries(*patterns: str) -> dict[pathlib.Path, str]: + """Build a `{target: pattern}` entries dict from CODEOWNERS pattern strings.""" + return {Path(pattern.strip('/')): pattern for pattern in patterns} + + +# The real repo always has a top-level `interfaces/` directory (with `interfaces/foo`), so +# `check` indexes into both unconditionally. These provide a fully-owned baseline that tests +# extend, so a test only needs to specify the paths and entries it actually exercises. +_BASE_FILES = ('interfaces/foo/ruff.toml',) +_BASE_PATTERNS = ('/interfaces/', '/interfaces/foo/') class TestParseCodeowners: def test_keys_by_repo_relative_target(self): text = '/apt/ @canonical/team # trailing comment\n' - assert check_codeowners.parse_codeowners(text) == { - PurePath('apt'): Entry(pattern='/apt/', owners=('@canonical/team',)) - } + assert check_codeowners.parse_codeowners(text) == {Path('apt'): '/apt/'} def test_ignores_comments_and_blanks(self): text = '# a comment\n\n/apt/ @canonical/team\n' - assert list(check_codeowners.parse_codeowners(text)) == [PurePath('apt')] + assert check_codeowners.parse_codeowners(text) == {Path('apt'): '/apt/'} - def test_multiple_owners(self): + def test_keeps_pattern_verbatim(self): text = '/foo/ @canonical/one @canonical/two\n' - assert check_codeowners.parse_codeowners(text)[PurePath('foo')] == Entry( - pattern='/foo/', owners=('@canonical/one', '@canonical/two') - ) + assert check_codeowners.parse_codeowners(text)[Path('foo')] == '/foo/' def test_entry_without_owner(self): text = '/interfaces/index.json\n' - assert check_codeowners.parse_codeowners(text)[PurePath('interfaces/index.json')] == Entry( - pattern='/interfaces/index.json', owners=() - ) + assert check_codeowners.parse_codeowners(text) == { + Path('interfaces/index.json'): '/interfaces/index.json' + } def test_drops_wildcard_patterns(self): text = '* @canonical/team\n/*.md @canonical/team\n/foo/[abc] @t\n/apt/ @canonical/team\n' # Only the anchored, wildcard-free entry survives. - assert list(check_codeowners.parse_codeowners(text)) == [PurePath('apt')] + assert check_codeowners.parse_codeowners(text) == {Path('apt'): '/apt/'} def test_empty(self): assert check_codeowners.parse_codeowners('') == {} -class TestChildrenOf: - def test_root_children(self): - files = _files('README.md', 'apt/pyproject.toml', 'apt/src/__init__.py') - assert check_codeowners.children_of(check_codeowners.ROOT, files) == _files( - 'README.md', 'apt' - ) - - def test_directory_children(self): - files = _files('apt/pyproject.toml', 'apt/src/__init__.py') - assert check_codeowners.children_of(PurePath('apt'), files) == _files( - 'apt/pyproject.toml', 'apt/src' - ) - - def test_no_children_for_unrelated_dir(self): - files = _files('apt/x.py') - assert check_codeowners.children_of(PurePath('snap'), files) == [] - - -class TestRender: - def test_directory_gets_trailing_slash(self): - files = _files('apt/x.py') - assert check_codeowners.render(PurePath('apt'), files) == '/apt/' - - def test_file_has_no_trailing_slash(self): - files = _files('README.md') - assert check_codeowners.render(PurePath('README.md'), files) == '/README.md' - - -class TestFindBadEntries: - def test_existing_paths_are_not_bad(self, tmp_path: pathlib.Path): - (tmp_path / 'apt').mkdir() - (tmp_path / 'interfaces' / 'foo').mkdir(parents=True) - (tmp_path / 'interfaces' / 'foo' / 'ruff.toml').touch() - entries = _entries('/apt/', '/interfaces/foo/', '/interfaces/foo/ruff.toml') - assert check_codeowners.find_bad_entries(entries, tmp_path) == [] +class TestMapChildren: + def test_maps_parents_to_immediate_children(self): + children = _children('README.md', 'apt/pyproject.toml', 'apt/src/__init__.py') + assert children == { + Path(): [Path('README.md'), Path('apt')], + Path('apt'): [Path('apt/pyproject.toml'), Path('apt/src')], + Path('apt/src'): [Path('apt/src/__init__.py')], + } - def test_missing_path_is_bad(self, tmp_path: pathlib.Path): - (tmp_path / 'apt').mkdir() - entries = _entries('/apt/', '/gone/') - bad = check_codeowners.find_bad_entries(entries, tmp_path) - assert [e.pattern for e in bad] == ['/gone/'] + def test_files_are_not_keys(self): + children = _children('README.md') + assert children == {Path(): [Path('README.md')]} + assert Path('README.md') not in children - def test_directory_entry_without_trailing_slash_is_bad(self, tmp_path: pathlib.Path): - (tmp_path / 'apt').mkdir() - entries = _entries('/apt') # missing the trailing slash - bad = check_codeowners.find_bad_entries(entries, tmp_path) - assert [e.pattern for e in bad] == ['/apt'] + def test_empty(self): + assert _children() == {} - def test_file_entry_with_trailing_slash_is_bad(self, tmp_path: pathlib.Path): - (tmp_path / 'README.md').touch() - entries = _entries('/README.md/') # stray trailing slash on a file - bad = check_codeowners.find_bad_entries(entries, tmp_path) - assert [e.pattern for e in bad] == ['/README.md/'] +class TestCheck: + def _check(self, entries: dict[pathlib.Path, str], *files: str) -> list[str]: + """Run `check` with a fully-owned `interfaces/` baseline plus the given entries/files.""" + return check_codeowners.check( + {**_entries(*_BASE_PATTERNS), **entries}, + _children(*_BASE_FILES, *files), + ) -class TestFindUnownedDirs: def test_path_with_own_entry_passes(self): - entries = _entries('/apt/') - files = _files('apt/src/x.py') - assert check_codeowners.find_unowned_dirs(entries, files) == [] - - def test_trailing_slash_is_ignored_when_matching(self): - # Matching is slash-insensitive (PurePosixPath); find_bad_entries enforces the slash. - entries = _entries('/apt') # no trailing slash, but still matches the apt directory - files = _files('apt/src/x.py') - assert check_codeowners.find_unowned_dirs(entries, files) == [] + assert self._check(_entries('/apt/'), 'apt/src/x.py') == [] def test_all_children_owned_passes(self): - # Split interface entries: interfaces/foo has no entry, but each of its children does. - entries = _entries( - '/interfaces/', '/interfaces/foo/interface/', '/interfaces/foo/ruff.toml' - ) - files = _files('interfaces/foo/interface/v0/schema.py', 'interfaces/foo/ruff.toml') - assert check_codeowners.find_unowned_dirs(entries, files) == [] - - def test_all_children_disowned_passes(self): - # Children with ownerless entries are explicitly disowned, which satisfies the parent. - entries = { - PurePath('interfaces'): Entry('/interfaces/', ('@canonical/team',)), - PurePath('interfaces/foo/interface'): Entry('/interfaces/foo/interface/', ()), - PurePath('interfaces/foo/ruff.toml'): Entry('/interfaces/foo/ruff.toml', ()), - } - files = _files('interfaces/foo/interface/v0/schema.py', 'interfaces/foo/ruff.toml') - assert check_codeowners.find_unowned_dirs(entries, files) == [] + # Split interface entries: interfaces/bar has no entry, but each of its children does. + entries = _entries('/interfaces/bar/interface/', '/interfaces/bar/ruff.toml') + files = ('interfaces/bar/interface/v0/schema.py', 'interfaces/bar/ruff.toml') + assert self._check(entries, *files) == [] - def test_some_children_unowned_is_unowned(self): - # /interfaces/ is owned; only interfaces/foo (whose ruff.toml lacks an entry) is unowned. - entries = _entries('/interfaces/', '/interfaces/foo/interface/') - files = _files('interfaces/foo/interface/v0/schema.py', 'interfaces/foo/ruff.toml') - assert check_codeowners.find_unowned_dirs(entries, files) == [PurePath('interfaces/foo')] + def test_top_level_file_needs_own_entry(self): + # The unowned top-level README.md is reported. + problems = self._check({}, 'README.md') + assert problems == ['No explicit CODEOWNERS for: /README.md'] + + def test_dir_with_unowned_child_is_reported(self): + # interfaces/bar's ruff.toml lacks an entry, so bar is unowned. + entries = _entries('/interfaces/bar/interface/') + files = ('interfaces/bar/interface/v0/schema.py', 'interfaces/bar/ruff.toml') + assert self._check(entries, *files) == [ + "No explicit CODEOWNERS for: /interfaces/bar/ (and its children aren't all owned)" + ] def test_rule_is_not_recursive(self): - # A grandchild entry must NOT satisfy a child; `interfaces/foo/interface` has no entry, - # only its own child does, so `interfaces/foo` is not owned. - entries = _entries('/interfaces/', '/interfaces/foo/interface/v0/') - files = _files('interfaces/foo/interface/v0/schema.py') - assert check_codeowners.find_unowned_dirs(entries, files) == [PurePath('interfaces/foo')] - - def test_no_entries_means_unowned(self): - # The catch-all `*` is dropped by the parser, so find_unowned_dirs never sees it. - files = _files('apt/src/x.py') - assert check_codeowners.find_unowned_dirs({}, files) == [PurePath('apt')] + # A grandchild entry must NOT satisfy a child; interfaces/bar/interface has no entry. + entries = _entries('/interfaces/bar/interface/v0/') + assert self._check(entries, 'interfaces/bar/interface/v0/schema.py') == [ + "No explicit CODEOWNERS for: /interfaces/bar/ (and its children aren't all owned)" + ] def test_parent_dir_entry_does_not_own_child(self): - # `/interfaces/` (the fallback) must not be treated as owning a specific interface. - entries = _entries('/interfaces/') - files = _files('interfaces/foo/ruff.toml') - assert check_codeowners.find_unowned_dirs(entries, files) == [PurePath('interfaces/foo')] - - def test_top_level_file_needs_own_entry(self): - entries = _entries('/other') - files = _files('README.md') - assert check_codeowners.find_unowned_dirs(entries, files) == [PurePath('README.md')] + # /interfaces/ owns the interfaces dir, but not a specific interface inside it. + assert self._check({}, 'interfaces/bar/ruff.toml') == [ + "No explicit CODEOWNERS for: /interfaces/bar/ (and its children aren't all owned)" + ] + + def test_missing_entry_target_is_reported(self): + problems = self._check(_entries('/apt/', '/gone/'), 'apt/src/x.py') + assert 'CODEOWNERS entry points at a missing path: /gone/' in problems + + def test_directory_entry_without_trailing_slash_is_reported(self): + problems = self._check(_entries('/apt'), 'apt/src/x.py') # missing trailing slash + assert 'CODEOWNERS entry must have a trailing slash iff it is a dir: /apt' in problems + + def test_file_entry_with_trailing_slash_is_reported(self): + problems = self._check(_entries('/README.md/'), 'README.md') # stray trailing slash + msg = 'CODEOWNERS entry must have a trailing slash iff it is a dir: /README.md/' + assert msg in problems + + def test_unanchored_entry_is_reported(self): + # A pattern without a leading slash is forbidden (parsed relative to the repo root). + problems = self._check({Path('apt'): 'apt/'}, 'apt/src/x.py') + assert 'CODEOWNERS entry must be anchored with a leading /: apt/' in problems From 6ffcc2e3f291f7baa5edea4530b8fc3664b0e54b Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 20:08:56 +1200 Subject: [PATCH 17/19] ci: add codeowners entry for code of conduct --- CODEOWNERS | 1 + 1 file changed, 1 insertion(+) diff --git a/CODEOWNERS b/CODEOWNERS index fcb66edb8..81d9dba74 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -12,6 +12,7 @@ /.template/ @canonical/charmlibs-maintainers /.workshop/ @canonical/charmlibs-maintainers /AGENTS.md @canonical/charmlibs-maintainers +/CODE_OF_CONDUCT.md @canonical/charmlibs-maintainers /CONTRIBUTING.md @canonical/charmlibs-maintainers /docs.just @canonical/charmlibs-maintainers /interface-test-requirements.txt @canonical/charmlibs-maintainers From a58ce1b4325b22bbf43ebfe0bf23cd10946cd0b2 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 20:15:24 +1200 Subject: [PATCH 18/19] ci: pin action by hash --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 1d6dcb9e8..d71a9212a 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -131,7 +131,7 @@ jobs: - uses: actions/checkout@v6 with: persist-credentials: false - - uses: astral-sh/setup-uv@v7 + - uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7 - name: Ensure every package and interface has a CODEOWNERS entry, and no entries are orphaned run: uvx --from rust-just just check-codeowners From 4519f6aaa3b62d3c58ca8ec041ab62b8254cb3e4 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 13 Jul 2026 20:17:24 +1200 Subject: [PATCH 19/19] ci: pin action --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d71a9212a..fb1b66274 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -128,7 +128,7 @@ jobs: codeowners: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: persist-credentials: false - uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7