Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/PULL_REQUEST_TEMPLATE/adding-a-new-library.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ Package:

Repository metadata:
- [ ] `.docs/reference/libs.yaml` updated with a new entry.
- [ ] `CODEOWNERS` updated with a `/<package>/` entry for the owning team.

Tests and docs:
- [ ] Unit tests added, plus functional and integration tests as appropriate.
Expand Down
1 change: 0 additions & 1 deletion .github/PULL_REQUEST_TEMPLATE/migrating-a-library.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ Package:

Repository metadata:
- [ ] `.docs/reference/libs.yaml` updated with entries for new and old libs.
- [ ] `CODEOWNERS` updated with a `/<package>/` entry for the owning team.

Tests and docs:
- [ ] Unit tests migrated, plus functional and integration tests as appropriate.
Expand Down
10 changes: 10 additions & 0 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
130 changes: 130 additions & 0 deletions .scripts/check_codeowners.py
Original file line number Diff line number Diff line change
@@ -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 :)')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the old-school smiley to distinguish from the AI favoured emoji ✅

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we avoiding Latin in this repo?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not very well -- some ag searching shows:

{
  "e.g.": 102,
  "i.e.": 16,
  "etc.": 13,
  "verbatim": 5,
  "vice versa": 1,
  "ergo": 1
}

But on the bright side, no hits for:

[
  "N.B.",
  "a priori",
  "ad hoc",
  "ad hominem",
  "bona fide",
  "cf.",
  "de facto",
  "de jure",
  "et al.",
  "et cetera",
  "et seq.",
  "ibid.",
  "in situ",
  "in vitro",
  "inter alia",
  "mutatis mutandis",
  "per se",
  "quid pro quo",
  "sic",
  "sine qua non",
  "status quo",
  "viz.",
  "vs."
]

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always a bit iffy (heh) about using "iff". This is an internal script so it probably doesn't matter, but I wonder how widely people understand it versus thinking someone has typo'd "if".

`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}'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, though this is public. I think the one above could be left in place but we could spell it out here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll split this into two types of problems: directory with no /, and non-directory with /.

)
if not pattern.startswith('/'):
problems.append(f'CODEOWNERS entry must be anchored with a leading /: {pattern}')
return problems


if __name__ == '__main__':
sys.exit(main())
7 changes: 7 additions & 0 deletions .scripts/just.py
Original file line number Diff line number Diff line change
Expand Up @@ -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/`."""
Expand Down
148 changes: 148 additions & 0 deletions .scripts/tests/test_check_codeowners.py
Original file line number Diff line number Diff line change
@@ -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
Loading
Loading