Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
3 changes: 2 additions & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ jobs:
steps:
- uses: actions/checkout@v6
- uses: actions/setup-python@v6
- run: pip install -r requirements.txt
- run: pip install -r requirements-dev.txt
- run: pip install .
- run: pytest
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
__pycache__
mise.toml
herald_rules.json
build/
*.egg-info
40 changes: 30 additions & 10 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,57 @@ Select reviewers based on a diff and a set of rules.
Requirements: [uv](https://docs.astral.sh/uv/#installation).

$ uv venv
$ uv pip install -r requirements.txt
$ uv pip install -e .


# Running

In its simplest form, the script accepts a diff on stdin. It processes the diff
according to a rule file passed as an argument, and outputs a list of
individual and groups of reviewers.

$ uv run ./reviewer_selector.py herald_rules.sample.json < sample.diff
$ uv run reviewer-selector samples/herald_rules.sample.json < samples/sample.diff
#example-group shtrom

The group prefix can be changed with `--group-prefix`. The reviewer separator
can be changed with `--reviewer-separator`. The `--repo` option allows the user
to specify a specific repository (to be used when evaluating conditions in some
rules.


# WARNING: The rules format is a work in progress

The current rules files, as shown in [the sample](./herald_rules.sample.json)
is not final and not normative. It is used as a bootstrapping stop-gap, and
should not be expected to remain stable at this stage.
The current rules files, as shown in [the
sample](./samples/herald_rules.sample.json) is not final and not normative. It
is used as a bootstrapping stop-gap, and should not be expected to remain
stable at this stage.


# Development tasks

# Tests
All the commands in this section rely on the development dependencies being installed.

$ uv pip install -e .[dev]

## Tests

$ uv run pytest

# Linting
## Linting

$ uv run ruff format

## Regenerating requirements.txt

Runtime dependencies only.

$ uv run pip-compile --quiet --generate-hashes --allow-unsafe -o requirements.txt

Include dev and testing dependencies.

$ uv run pip-compile --quiet --generate-hashes --extra=dev --allow-unsafe -o requirements-dev.txt


# Containerised deployment

For use as part of a more complex pipeline, a docker image can be built. It is
Expand All @@ -47,16 +68,15 @@ a GitHub pull request directly if enough information is available.

Requirements: [docker](https://docs.docker.com/get-started/get-docker/).

$ docker build -t reviewer-selector .

$ docker build -f docker/Dockerfile -t reviewer-selector .

## Running in a container

For convenience, the sample rules are shipped with the container image. The
reviewers string is formatted for use with [GitHub's gh
CLI](https://cli.github.com/).

$ docker run --rm -i reviewer-selector < sample.diff
$ docker run --rm -i reviewer-selector < samples/sample.diff
No DIFF_URL in environment, reading from stdin ...
No PR_URL or GITHUB_TOKEN in environment, outputing to stdout ...
@example-group,shtrom
Expand Down
6 changes: 3 additions & 3 deletions Dockerfile → docker/Dockerfile
Copy link
Member

Choose a reason for hiding this comment

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

If this Docker container is used for running tests, perhaps we should use it in the GHA workflow instead of installing requirements-dev.txt and running pytest directly.

Copy link
Member Author

@shtrom shtrom Mar 16, 2026

Choose a reason for hiding this comment

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

This container is the main artifact that gets run by the TC hook. It's very very lean at the moment, but yeah... I'll see if I can keep the test dependencies out of the container.

Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ RUN apt-get -y update \
RUN mkdir /app
WORKDIR /app

RUN --mount=type=bind,src=requirements.txt,target=/app/requirements.txt pip install -r requirements.txt
RUN --mount=type=bind,src=../requirements.txt,target=/app/requirements.txt pip install -r requirements.txt

COPY reviewer_selector.py entrypoint.sh /app/
COPY ../src/reviewer_selector.py ../docker/entrypoint.sh /app/
# Example for self-contained quick tests.
COPY herald_rules.sample.json /app/herald_rules.json
COPY ../samples/herald_rules.sample.json /app/herald_rules.json

CMD [ "herald_rules.json" ]

Expand Down
File renamed without changes.
29 changes: 29 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,2 +1,31 @@
[project]
classifiers = [
"Development Status :: 3 - Alpha",
"Intended Audience :: Developers",
"Operating System :: OS Independent",
"Programming Language :: Python :: 3",
]
dependencies= [
"rs_parsepatch",
]
description = "Select reviewers based on a diff and a set of rules"
dynamic = ["version"]
Copy link
Member

Choose a reason for hiding this comment

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

I think this field only needs to be set when using setuptools-scm. Perhaps we could add that now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it would, and was surprised that it... just worked? (TBF, I haven't checked the version anywhere, but it just didn't choke on it).

I'm not sure we need setuptools at all considering we don't build sdist-type files.

OTOH, I have just tried adding a tag now, and the version is still 0.0.0 with or without setuptools-scm.

license = "MPL-2.0"
maintainers = [
{name = "Mozilla Conduit", email = "conduit-team@mozilla.com"},
]
name = "reviewer-selector"
requires-python = ">=3.12"

[project.optional-dependencies]
dev = [
"pip-tools",
"pytest",
"pytest-ruff",
Copy link
Member

Choose a reason for hiding this comment

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

pytest-ruff looks like a small, mostly unmaintained project (last activity 9 months ago, adding Python 3.13 support). We should roll our own formatting tests as we do in other projects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it's just feature complete? 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Lando uses both Black and Ruff. Why do we use both and not just one?

]

[project.scripts]
reviewer-selector = "reviewer_selector:main"

[tool.pytest]
addopts = ["--ruff", "--ruff-format" ]
95 changes: 95 additions & 0 deletions requirements-dev.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
#
# This file is autogenerated by pip-compile with Python 3.12
# by the following command:
#
# pip-compile --allow-unsafe --extra=dev --generate-hashes --output-file=requirements-dev.txt
#
build==1.4.0 \
--hash=sha256:6a07c1b8eb6f2b311b96fcbdbce5dab5fe637ffda0fd83c9cac622e927501596 \
--hash=sha256:f1b91b925aa322be454f8330c6fb48b465da993d1e7e7e6fa35027ec49f3c936
# via pip-tools
click==8.3.1 \
--hash=sha256:12ff4785d337a1bb490bb7e9c2b1ee5da3112e94a8622f26a6c77f5d2fc6842a \
--hash=sha256:981153a64e25f12d547d3426c367a4857371575ee7ad18df2a6183ab0545b2a6
# via pip-tools
iniconfig==2.3.0 \
--hash=sha256:c76315c77db068650d49c5b56314774a7804df16fee4402c1f19d6d15d8c4730 \
--hash=sha256:f631c04d2c48c52b84d0d0549c99ff3859c98df65b3101406327ecc7d53fbf12
# via pytest
packaging==26.0 \
--hash=sha256:00243ae351a257117b6a241061796684b084ed1c516a08c48a3f7e147a9d80b4 \
--hash=sha256:b36f1fef9334a5588b4166f8bcd26a14e521f2b55e6b9de3aaa80d3ff7a37529
# via
# build
# pytest
# wheel
pip-tools==7.5.3 \
--hash=sha256:3aac0c473240ae90db7213c033401f345b05197293ccbdd2704e52e7a783785e \
--hash=sha256:8fa364779ebc010cbfe17cb9de404457ac733e100840423f28f6955de7742d41
# via reviewer-selector (pyproject.toml)
pluggy==1.6.0 \
--hash=sha256:7dcc130b76258d33b90f61b658791dede3486c3e6bfb003ee5c9bfb396dd22f3 \
--hash=sha256:e920276dd6813095e9377c0bc5566d94c932c33b27a3e3945d8389c374dd4746
# via pytest
pygments==2.19.2 \
--hash=sha256:636cb2477cec7f8952536970bc533bc43743542f70392ae026374600add5b887 \
--hash=sha256:86540386c03d588bb81d44bc3928634ff26449851e99741617ecb9037ee5ec0b
# via pytest
pyproject-hooks==1.2.0 \
--hash=sha256:1e859bd5c40fae9448642dd871adf459e5e2084186e8d2c2a79a824c970da1f8 \
--hash=sha256:9e5c6bfa8dcc30091c74b0cf803c81fdd29d94f01992a7707bc97babb1141913
# via
# build
# pip-tools
pytest==9.0.2 \
--hash=sha256:711ffd45bf766d5264d487b917733b453d917afd2b0ad65223959f59089f875b \
--hash=sha256:75186651a92bd89611d1d9fc20f0b4345fd827c41ccd5c299a868a05d70edf11
# via
# pytest-ruff
# reviewer-selector (pyproject.toml)
pytest-ruff==0.5 \
--hash=sha256:d9db170d86fb167008e6702b4d79e2cccd8287f069c3a57f9261831cebdc4a31 \
--hash=sha256:f611c780fc2b9b8d7041fa0e7589f0a9f352b288d0cfc330881101b35d382063
# via reviewer-selector (pyproject.toml)
rs-parsepatch==0.4.6 \
--hash=sha256:5a0fd2b857dd9fe5ee987540f675220e5088e7bec16f2ffa8a12968b9c1a040f \
--hash=sha256:892259ef2c6feacddcdee5d56b92ef698249d261efbc3a8df4a6e5bcc90e61e1 \
--hash=sha256:b3a1269b5b54d90c0346f4959eab996c12d315bfd7a7d46f366205b89fd44536 \
--hash=sha256:b4b6559077643f63c79f0c30de4815ee5f5226fa3d1f140ca64b778d7a127edc \
--hash=sha256:c7be548dfb16dafa7fc2b2cc304d319a70631332c55a5175d859dfda6daeb9c6 \
--hash=sha256:d9b13521edce9c6f176ecb07c437789a610697033239507b8bea7a7d1cf6c55b
# via reviewer-selector (pyproject.toml)
ruff==0.15.6 \
--hash=sha256:13f4594b04e42cd24a41da653886b04d2ff87adbf57497ed4f728b0e8a4866f8 \
--hash=sha256:1c22e6f02c16cfac3888aa636e9eba857254d15bbacc9906c9689fdecb1953ab \
--hash=sha256:3bd9967851a25f038fc8b9ae88a7fbd1b609f30349231dffaa37b6804923c4bb \
--hash=sha256:542aaf1de3154cea088ced5a819ce872611256ffe2498e750bbae5247a8114e9 \
--hash=sha256:55a1ad63c5a6e54b1f21b7514dfadc0c7fb40093fa22e95143cf3f64ebdcd512 \
--hash=sha256:70789d3e7830b848b548aae96766431c0dc01a6c78c13381f423bf7076c66d15 \
--hash=sha256:70d263770d234912374493e8cc1e7385c5d49376e41dfa51c5c3453169dc581c \
--hash=sha256:7c98c3b16407b2cf3d0f2b80c80187384bc92c6774d85fefa913ecd941256fff \
--hash=sha256:8394c7bb153a4e3811a4ecdacd4a8e6a4fa8097028119160dffecdcdf9b56ae4 \
--hash=sha256:85b042377c2a5561131767974617006f99f7e13c63c111b998f29fc1e58a4cfb \
--hash=sha256:8dc473ba093c5ec238bb1e7429ee676dca24643c471e11fbaa8a857925b061c0 \
--hash=sha256:98893c4c0aadc8e448cfa315bd0cc343a5323d740fe5f28ef8a3f9e21b381f7e \
--hash=sha256:aee25bc84c2f1007ecb5037dff75cef00414fdf17c23f07dc13e577883dca406 \
--hash=sha256:bbf67d39832404812a2d23020dda68fee7f18ce15654e96fb1d3ad21a5fe436c \
--hash=sha256:c34de3dd0b0ba203be50ae70f5910b17188556630e2178fd7d79fc030eb0d837 \
--hash=sha256:cef49e30bc5a86a6a92098a7fbf6e467a234d90b63305d6f3ec01225a9d092e0 \
--hash=sha256:e2ed8aea2f3fe57886d3f00ea5b8aae5bf68d5e195f487f037a955ff9fbaac9e \
--hash=sha256:ee7dcfaad8b282a284df4aa6ddc2741b3f4a18b0555d626805555a820ea181c3
# via pytest-ruff
wheel==0.46.3 \
--hash=sha256:4b399d56c9d9338230118d705d9737a2a468ccca63d5e813e2a4fc7815d8bc4d \
--hash=sha256:e3e79874b07d776c40bd6033f8ddf76a7dad46a7b8aa1b2787a83083519a1803
# via pip-tools

# The following packages are considered to be unsafe in a requirements file:
pip==26.0.1 \
--hash=sha256:bdb1b08f4274833d62c1aa29e20907365a2ceb950410df15fc9521bad440122b \
--hash=sha256:c4037d8a277c89b320abe636d59f91e6d0922d08a05b60e85e53b296613346d8
# via pip-tools
setuptools==82.0.1 \
--hash=sha256:7d872682c5d01cfde07da7bccc7b65469d3dca203318515ada1de5eda35efbf9 \
--hash=sha256:a59e362652f08dcd477c78bb6e7bd9d80a7995bc73ce773050228a348ce2e5bb
# via pip-tools
17 changes: 14 additions & 3 deletions requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

As this package is intended to be a library/client-side CLI tool (as opposed to server-side software) I don't think we need a requirements.txt. Specifying the requirements in pyproject.toml will allow the build to find packages that work for the client given other venv conflicts, as long as we ensure we specify any incompatible versions.

requirements-dev.txt is still valid for running local tests and builds against a known-good version.

Copy link
Member Author

@shtrom shtrom Mar 16, 2026

Choose a reason for hiding this comment

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

I think it's gonna be server-side: we build a docker container that will be used in response to hooks. If anything, as we refine the reviewer selection, this will be used as the shell to potentially bring in other libraries, but not the other way round.

It is, however, designed as a CLI for ease of testing in isolation, but I think having the requirements.txt here helps too, to recreate a known-good environment for the CLI.

Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
rs_parsepatch
pytest
pytest-ruff
#
# This file is autogenerated by pip-compile with Python 3.12
# by the following command:
#
# pip-compile --allow-unsafe --generate-hashes --output-file=requirements.txt
#
rs-parsepatch==0.4.6 \
--hash=sha256:5a0fd2b857dd9fe5ee987540f675220e5088e7bec16f2ffa8a12968b9c1a040f \
--hash=sha256:892259ef2c6feacddcdee5d56b92ef698249d261efbc3a8df4a6e5bcc90e61e1 \
--hash=sha256:b3a1269b5b54d90c0346f4959eab996c12d315bfd7a7d46f366205b89fd44536 \
--hash=sha256:b4b6559077643f63c79f0c30de4815ee5f5226fa3d1f140ca64b778d7a127edc \
--hash=sha256:c7be548dfb16dafa7fc2b2cc304d319a70631332c55a5175d859dfda6daeb9c6 \
--hash=sha256:d9b13521edce9c6f176ecb07c437789a610697033239507b8bea7a7d1cf6c55b
# via reviewer-selector (pyproject.toml)
File renamed without changes.
File renamed without changes.
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
resolve_reviewers,
)

MAIN_SCRIPT = "src/reviewer_selector.py"
Copy link
Member

Choose a reason for hiding this comment

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

If you want to test the script end-to-end, we should simply execute the name as defined in pyproject.toml:project.scripts (ie subprocess.run(["reviewer-selector"... in this case). Alternatively we could run the main() method directly. Executing the script in this way is brittle as changing the project structure will cause unrelated test failures. It also doesn't reflect any changes to the environment that may happen as part of the build/install.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go with the installed script name, rather than the main, to be sure to test the whole flow (including correct installation of the script).


# --- Test data ---

Expand Down Expand Up @@ -367,7 +368,7 @@ def test_full_flow(self):
rules_path = f.name

result = subprocess.run(
[sys.executable, "reviewer_selector.py", rules_path],
[sys.executable, MAIN_SCRIPT, rules_path],
input=SAMPLE_DIFF,
capture_output=True,
text=True,
Expand All @@ -393,7 +394,7 @@ def test_repo_filter(self):
result = subprocess.run(
[
sys.executable,
"reviewer_selector.py",
MAIN_SCRIPT,
rules_path,
"--repo",
"mozilla-central",
Expand All @@ -412,7 +413,7 @@ def test_group_prefix(self):
rules_path = f.name

result = subprocess.run(
[sys.executable, "reviewer_selector.py", rules_path, "--group-prefix", "@"],
[sys.executable, MAIN_SCRIPT, rules_path, "--group-prefix", "@"],
input=SAMPLE_DIFF,
capture_output=True,
text=True,
Expand Down
Loading