Skip to content
Open
Show file tree
Hide file tree
Changes from 11 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
7 changes: 4 additions & 3 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
- uses: actions/setup-python@v6
- run: pip install -r requirements.txt
- run: pytest
- run: docker build -f docker/Dockerfile -t reviewer-selector .
- run: >
docker run --entrypoint bash reviewer-selector
-c 'pip install -r requirements-dev.txt && 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
64 changes: 50 additions & 14 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,35 +7,56 @@ 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 check --fix

## 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

$ uv run ruff format

# Containerised deployment

Expand All @@ -47,17 +68,19 @@ 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 REPO_URL in environment, using built-in rules ...
No DIFF_URL in environment, reading from stdin ...
No ORG_NAME in environment, using # as group prefix ...
No REPO_NAME or TARGET_BRANCH_NAME in environment, not matching repository-based rules ...
No PR_URL or GITHUB_TOKEN in environment, outputing to stdout ...
@example-group,shtrom

Expand All @@ -71,7 +94,10 @@ piped into the container.
| docker run --rm -i \
-v ./herald_rules.real.json:/app/herald_rules.json \
reviewer-selector
No REPO_URL in environment, using built-in rules ...
No DIFF_URL in environment, reading from stdin ...
No ORG_NAME in environment, using # as group prefix ...
No REPO_NAME or TARGET_BRANCH_NAME in environment, not matching repository-based rules ...
No PR_URL or GITHUB_TOKEN in environment, outputing to stdout ...
@android-reviewers

Expand All @@ -84,6 +110,16 @@ The container's behaviour can be entirely parametrised via environment variables
-e GITHUB_TOKEN=[REDACTED] reviewer-selector
Adding reviewers to https://github.com/mozilla-firefox/infra-testing/pull/30 ...

If `DIFF_URL` is given, it will be fetched and passed into the selector's
stdin. If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt
to set the reviewers on the target PR.
* If `DIFF_URL` is given, it will be fetched and passed into the selector's
stdin.
* If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt
* If `GITHUB_TOKEN` and `PR_URL` are provided, the container will attempt to set the reviewers on the target PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! This one travelled all the way to the bottom of the file!


Some other optional behaviours can be triggered by providing additional context
in environment variables:
* If `ORG_NAME` is passed, it will be used to scope reviewers groups to that
org.
* If `REPO_NAME` and `TARGET_BRANCH_NAME` are provided, rules that specifically
match a given repository and/or branch will also be applied.
* If `REPO_URL` is given, the script will attempt to fetch a rules file from
the `main` branch of the repository. If it fails, it will fallback to
built-in rules. to set the reviewers on the target PR.
8 changes: 5 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,13 @@ 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
COPY . /app
RUN pip install -r requirements.txt \
&& pip install /app
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it would be better to COPY the requirements file in and build the venv first, then copy the app code. That way we only re-build the container when the requirements.txt file changes, instead of each time the app code changes.

This is a nit as it's not super important for an MVP project. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I didn't think of that. In a previous installment I had a RUN --mount, but I removed it when I ended up COPYing everything. I'll restore that!


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
73 changes: 73 additions & 0 deletions docker/entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
#!/bin/sh -eu
Copy link
Member

Choose a reason for hiding this comment

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

Small non-blocking request: could I trouble you to convert this into an entrypoint.py instead? As our team's stack is Python, it seems it would be easier to maintain. I can generally follow what's happening in this script, but if there was a subtle shell-script bug here I would have a hard time spotting it. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yeah. That shell script kinda grew a bit in size. I think the rewrite will happen naturally in the next few PR as I integrate TC and GitHub, and Python code ultimately becomes a shell around mots.

# Variables expected in environment:
# * DIFF_URL (optional, will read stdin otherwise)
# * GITHUB_TOKEN (optional, will output reviewers to stdout otherwise)
# * PR_URL (optional, will output reviewers to stdout otherwise)
# * ORG_NAME (optional, will use # as a group prefix otherwise)
# * REPO_NAME (optional, for more rule matching with TARGET_BRANCH_NAME)
# * TARGET_BRANCH_NAME (optional, for more rule matching with REPO_NAME)
# * REPO_URL (optional, to fetch specific rules)

CURL="curl --fail --show-error --silent --location"

if [ -n "${REPO_URL:-}" ]; then
HERALD_RULES_JSON=$(mktemp)
# We always fetch the rules from the main branch.
RULES_URL="${REPO_URL}/refs/heads/main/herald_rules.json"

if ! ${CURL} "${RULES_URL}" --output "${HERALD_RULES_JSON}"; then
echo "Failed to fetch rules from ${RULES_URL}, using built-in rules ..." >&2
HERALD_RULES_JSON=""

fi

else
echo "No REPO_URL in environment, using built-in rules ..." >&2

fi

if [ -z "${HERALD_RULES_JSON:-}" ]; then
HERALD_RULES_JSON="${1}"
fi

DIFF=$(mktemp)
if [ -n "${DIFF_URL:-}" ]; then
${CURL} "${DIFF_URL}" --output "${DIFF}"

else
echo "No DIFF_URL in environment, reading from stdin ..." >&2
cat > "${DIFF}"

fi

if [ -n "${ORG_NAME:-}" ]; then
GROUP_PREFIX="${ORG_NAME}/"

else
GROUP_PREFIX="#"
echo "No ORG_NAME in environment, using ${GROUP_PREFIX} as group prefix ..." >&2

fi

if [ -n "${REPO_NAME:-}" ] && [ -n "${TARGET_BRANCH_NAME:-}" ]; then
REPO_BRANCH=${REPO_NAME}-${TARGET_BRANCH_NAME}

else
echo "No REPO_NAME or TARGET_BRANCH_NAME in environment, not matching repository-based rules ..." >&2
fi

REVIEWERS=$(cat "${DIFF}" \
| /app/reviewer_selector.py \
${REPO_BRANCH:+--repo "${REPO_BRANCH}"} \
--group-prefix "${GROUP_PREFIX}" --reviewer-separator , \
"${HERALD_RULES_JSON}" \
)

if [ -n "${GITHUB_TOKEN:-}" ] && [ -n "${PR_URL:-}" ]; then
echo "Adding reviewers to ${PR_URL} ..." >&2
gh pr edit "${PR_URL}" --add-reviewer "${REVIEWERS}"

else
echo "No PR_URL or GITHUB_TOKEN in environment, outputing to stdout ..." >&2
echo "${REVIEWERS}"
fi
29 changes: 0 additions & 29 deletions entrypoint.sh

This file was deleted.

35 changes: 33 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,2 +1,33 @@
[tool.pytest]
addopts = ["--ruff", "--ruff-format" ]
[build-system]
requires = ["setuptools>=80", "setuptools-scm>=8"]
build-backend = "setuptools.build_meta"

[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 = [
"black",
"pip-tools",
"pytest",
"ruff",
Comment on lines +26 to +29
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it might be worth pinning these to specific versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be the rationale here? We are hard-pinned by the requirements.txt, so my instinct would be to have nothing more than a loose pinning here, likely if we encounter incompatibility with newer versions that we can't fix immediately.

]

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