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..fb1b66274 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@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + - 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 + 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..9e58b62a9 --- /dev/null +++ b/.scripts/check_codeowners.py @@ -0,0 +1,130 @@ +#!/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 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 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 +and exit with failure (the number of problems found). +""" + +from __future__ import annotations + +import pathlib +import subprocess +import sys + +# `.scripts/check_codeowners.py` -> repo root is two parents up. +REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent + + +def main() -> int: + """Run both CODEOWNERS checks, printing any problems and returning the problem count.""" + entries = parse_codeowners((REPO_ROOT / 'CODEOWNERS').read_text()) + children = map_children(get_tracked_files(REPO_ROOT)) + problems = check(entries, children) + 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]: + """Parse CODEOWNERS into a `{target: pattern}` dict, keyed by the repo-relative path it covers. + + Comments, blanks, and wildcard patterns (containing any of `*?[]`) are ignored. + """ + entries: dict[pathlib.Path, str] = {} + for line in text.splitlines(): + entry, _, _ = line.partition('#') # drop trailing comments + entry = entry.strip() + if not entry: + continue + pattern, *_owners = entry.split() + if any(char in pattern for char in '*?[]'): + continue + target = pathlib.Path(pattern.strip('/')) # to be interpreted relative to REPO_ROOT + entries[target] = pattern + return entries + + +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 ('.'). + 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]: + """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 that each top-level path and immediate child of interfaces/ is owned. + for path in *children[pathlib.Path()], *children[pathlib.Path('interfaces')]: + 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(): + if target not in all_paths: + problems.append(f'CODEOWNERS entry points at a missing path: {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 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/.scripts/just.py b/.scripts/just.py index 5724276f1..dd4b05148 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 bad.""" + _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..547871a7a --- /dev/null +++ b/.scripts/tests/test_check_codeowners.py @@ -0,0 +1,148 @@ +# 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 + +check_codeowners = importlib.import_module('check_codeowners') +Path = pathlib.Path + + +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.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) == {Path('apt'): '/apt/'} + + def test_ignores_comments_and_blanks(self): + text = '# a comment\n\n/apt/ @canonical/team\n' + assert check_codeowners.parse_codeowners(text) == {Path('apt'): '/apt/'} + + def test_keeps_pattern_verbatim(self): + text = '/foo/ @canonical/one @canonical/two\n' + 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) == { + 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 check_codeowners.parse_codeowners(text) == {Path('apt'): '/apt/'} + + def test_empty(self): + assert check_codeowners.parse_codeowners('') == {} + + +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_files_are_not_keys(self): + children = _children('README.md') + assert children == {Path(): [Path('README.md')]} + assert Path('README.md') not in children + + def test_empty(self): + assert _children() == {} + + +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), + ) + + def test_path_with_own_entry_passes(self): + assert self._check(_entries('/apt/'), 'apt/src/x.py') == [] + + def test_all_children_owned_passes(self): + # 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_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/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/ 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 diff --git a/CODEOWNERS b/CODEOWNERS index 01e74cc9a..81d9dba74 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -7,11 +7,21 @@ # 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 +/CODE_OF_CONDUCT.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 @@ -22,6 +32,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,18 +97,53 @@ /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 # /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 @@ -114,6 +164,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,11 +275,19 @@ /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 # /interfaces/service_mesh/ -/interfaces/service_mesh @canonical/service-mesh +/interfaces/service_mesh/ @canonical/service-mesh # /interfaces/smtp/ /interfaces/smtp/interface/ @canonical/platform-engineering 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 "$@"