-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(ci): gate merges on maintainer approval via a required status check #3176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
af1a51d
2fb533c
635815f
587a57c
5c67728
b351c74
467a813
dad00ec
853e529
f92a2df
11c7836
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| name: Maintainer Approval | ||
|
|
||
| on: | ||
| merge_group: | ||
| types: [checks_requested] | ||
| pull_request_review: | ||
| types: [submitted, dismissed] | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: read | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| maintainer-approval: | ||
| # This name is the required status check context. Changing it silently | ||
| # breaks the ruleset entry that gates merges on this job. | ||
| name: OpenShell / Maintainer Approval | ||
| if: github.repository_owner == 'NVIDIA' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| steps: | ||
| # Check out the default branch, never the pull request head. Both the | ||
| # maintainer list and the decision helper must come from main: reading | ||
| # either from the pull request ref would let a contributor add themselves | ||
| # to the list, or rewrite the decision logic, and self-approve. | ||
| - name: Check out the maintainer list and helper | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| ref: main | ||
| sparse-checkout: | | ||
| MAINTAINERS.md | ||
| tasks/scripts/check_maintainer_approval.py | ||
| sparse-checkout-cone-mode: false | ||
| persist-credentials: false | ||
|
|
||
| - name: Require an approving review from a maintainer | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GH_REPO: ${{ github.repository }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| # merge_group carries no pull request in its payload, but the queue | ||
| # branch names the entry: gh-readonly-queue/main/pr-3027-<base sha>. | ||
| MERGE_GROUP_REF: ${{ github.ref_name }} | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| if [ -z "${PR_NUMBER:-}" ]; then | ||
| if [[ "$MERGE_GROUP_REF" =~ /pr-([0-9]+)-[0-9a-f]+$ ]]; then | ||
| PR_NUMBER="${BASH_REMATCH[1]}" | ||
| else | ||
| # Fail closed: an unrecognised ref must never satisfy the gate. | ||
| echo "::error::No pull request resolved from '$MERGE_GROUP_REF'." | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| gh api --paginate "repos/$GH_REPO/pulls/$PR_NUMBER/reviews" --jq '.[]' \ | ||
| | jq -s '.' > reviews.json | ||
|
|
||
| # Exits non-zero when no maintainer's latest decisive review is an | ||
| # approval, and when MAINTAINERS.md yields no handles. That exit code | ||
| # is the check result; nothing is posted anywhere. | ||
| python3 tasks/scripts/check_maintainer_approval.py \ | ||
| --maintainers MAINTAINERS.md \ | ||
| --reviews reviews.json |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| name: Maintainers Change Alert | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened, reopened, synchronize] | ||
| paths: | ||
| - MAINTAINERS.md | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| describe-change: | ||
| name: Comment on the approver set change | ||
| if: github.repository_owner == 'NVIDIA' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
| steps: | ||
| # Default branch only. The helper must be the reviewed version, not | ||
| # whatever the pull request happens to contain. | ||
| - name: Check out the change-alert helper | ||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| ref: main | ||
| sparse-checkout: tasks/scripts/alert_maintainer_change.py | ||
| sparse-checkout-cone-mode: false | ||
| persist-credentials: false | ||
|
|
||
| - name: Post the maintainer delta | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GH_REPO: ${{ github.repository }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| # Fetching file contents is reading data, not executing it. The head | ||
| # revision is never checked out or run. | ||
| # | ||
| # A 404 means the file genuinely does not exist at that revision — a | ||
| # pull request that adds or deletes MAINTAINERS.md — and yields an | ||
| # empty side of the comparison. Every other failure is fatal: an empty | ||
| # file from a rate limit or a 5xx would render as "every maintainer | ||
| # was just added" or "nothing changed", both of which mislead the | ||
| # reviewer about who can merge code. | ||
| fetch_maintainers() { | ||
| local ref="$1" out="$2" err | ||
| err="$(mktemp)" | ||
| if gh api -H "Accept: application/vnd.github.raw" \ | ||
| "repos/$GH_REPO/contents/MAINTAINERS.md?ref=$ref" > "$out" 2>"$err"; then | ||
| rm -f "$err" | ||
| return 0 | ||
| fi | ||
| if grep -q 'HTTP 404' "$err"; then | ||
| rm -f "$err" | ||
| : > "$out" | ||
| return 0 | ||
| fi | ||
| echo "::error::Could not fetch MAINTAINERS.md at $ref" | ||
| cat "$err" >&2 | ||
| rm -f "$err" | ||
| return 1 | ||
| } | ||
|
|
||
| fetch_maintainers "$BASE_SHA" before.md | ||
| fetch_maintainers "$HEAD_SHA" after.md | ||
|
|
||
| python3 tasks/scripts/alert_maintainer_change.py \ | ||
| --before before.md --after after.md > body.md | ||
| cat body.md >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| # Update the existing comment rather than stacking one per push. | ||
| # This marker must stay identical to COMMENT_MARKER in | ||
| # tasks/scripts/alert_maintainer_change.py, which emits it as the | ||
| # first line of the body. If they drift, the lookup below silently | ||
| # stops matching and every push stacks another comment. | ||
| COMMENT_ID=$(gh api --paginate "repos/$GH_REPO/issues/$PR_NUMBER/comments" \ | ||
| --jq '.[] | select(.body | startswith("<!-- maintainer-approval-delta -->")) | .id' \ | ||
| | head -n 1) | ||
|
|
||
| if [ -n "$COMMENT_ID" ]; then | ||
| gh api --method PATCH "repos/$GH_REPO/issues/comments/$COMMENT_ID" \ | ||
| -F "body=@body.md" >/dev/null | ||
| else | ||
| gh api --method POST "repos/$GH_REPO/issues/$PR_NUMBER/comments" \ | ||
| -F "body=@body.md" >/dev/null | ||
| fi | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| #!/usr/bin/env python3 | ||
| # /// script | ||
| # requires-python = ">=3.11" | ||
| # dependencies = [] | ||
| # /// | ||
|
|
||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Print a review comment describing how a pull request changes the approver set. | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "... to make sure that reviewers notice the change." |
||
|
|
||
| The calling workflow does the I/O: it extracts MAINTAINERS.md at the base and | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary comment. We can read this in the code. Comments say what we do and why concisely and only when the code doesn't speak for itself. |
||
| head commits, passes both as files, and posts the output as a comment. | ||
|
|
||
| Runs as bare `python3` on the Actions runner, so it must stay stdlib-only. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import re | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| # Only a login that appears as a link to a GitHub profile counts. A bare | ||
| # "[@someone]" in prose must never widen the approver set. Keep this in step | ||
| # with tasks/scripts/check_maintainer_approval.py, the gate this reports on. | ||
| MAINTAINER_RE = re.compile( | ||
| r"\[@([A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?)\]\(https://github\.com/" | ||
| ) | ||
|
|
||
| # The workflow finds its own earlier comment by this prefix, so it must stay | ||
| # identical on both sides. | ||
| COMMENT_MARKER = "<!-- maintainer-approval-delta -->" | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As noted elsewhere, would love to get this as an env var so we don't have to worry about coupling. |
||
|
|
||
|
|
||
| def parse_maintainers(markdown: str) -> set[str]: | ||
| """Return the lowercased GitHub logins listed in a MAINTAINERS.md table.""" | ||
| return {match.group(1).lower() for match in MAINTAINER_RE.finditer(markdown)} | ||
|
|
||
|
|
||
| def format_delta(before: str, after: str) -> str: | ||
| """Render a review comment describing how the approver set changes.""" | ||
| old, new = parse_maintainers(before), parse_maintainers(after) | ||
| added, removed = sorted(new - old), sorted(old - new) | ||
|
|
||
| lines = [COMMENT_MARKER, "## Maintainer list change", ""] | ||
| if not added and not removed: | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case we should not comment. It makes no value and unneeded noise is distracting. |
||
| lines.append( | ||
| "This pull request edits `MAINTAINERS.md` but does not change the set " | ||
| "of logins the approval gate recognises." | ||
| ) | ||
| else: | ||
| if added: | ||
| lines += ["**Gains approval rights:**", ""] | ||
| lines += [f"- @{login}" for login in added] | ||
| lines.append("") | ||
| if removed: | ||
| lines += ["**Loses approval rights:**", ""] | ||
| lines += [f"- @{login}" for login in removed] | ||
| lines.append("") | ||
| lines.append( | ||
| "Confirm every change is intended. Anyone listed here can single-handedly " | ||
| "satisfy `OpenShell / Maintainer Approval`." | ||
| ) | ||
|
|
||
| if not new: | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition should cause the check to fail. |
||
| lines += [ | ||
| "", | ||
| "> [!WARNING]", | ||
| "> No logins parse from the updated file. Merging this would make the " | ||
| "approval gate fail closed on every pull request.", | ||
| ] | ||
| return "\n".join(lines) | ||
|
|
||
|
|
||
| def main(argv: list[str] | None = None) -> int: | ||
| parser = argparse.ArgumentParser(description=__doc__) | ||
| parser.add_argument( | ||
| "--before", required=True, type=Path, help="MAINTAINERS.md at the base commit" | ||
| ) | ||
| parser.add_argument( | ||
| "--after", required=True, type=Path, help="MAINTAINERS.md at the head commit" | ||
| ) | ||
| args = parser.parse_args(argv) | ||
|
|
||
| print( | ||
| format_delta( | ||
| args.before.read_text(encoding="utf-8"), | ||
| args.after.read_text(encoding="utf-8"), | ||
| ) | ||
| ) | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Tests for tasks/scripts/alert_maintainer_change.py. | ||
|
|
||
| Run via `mise run test:maintainer-approval`, which provides pytest through | ||
| `uv run --with pytest`. pytest puts this file's directory on sys.path, so the | ||
| sibling script imports directly. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import alert_maintainer_change as alert | ||
|
|
||
| TABLE = """# Maintainers | ||
|
|
||
| | Name | GitHub ID | Company/Organization | | ||
| | --- | --- | --- | | ||
| | Derek Carr | [@derekwaynecarr](https://github.com/derekwaynecarr) | Red Hat | | ||
| | Evan Lezar | [@elezar](https://github.com/elezar) | NVIDIA | | ||
| | Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA | | ||
| """ | ||
|
|
||
|
|
||
| def test_parse_maintainers_extracts_linked_logins() -> None: | ||
| assert alert.parse_maintainers(TABLE) == {"derekwaynecarr", "elezar", "pimlock"} | ||
|
|
||
|
|
||
| def test_names_added_and_removed_logins() -> None: | ||
| after = TABLE.replace( | ||
| "| Piotr Mlocek | [@pimlock](https://github.com/pimlock) | NVIDIA |\n", | ||
| "| New Person | [@newbie](https://github.com/newbie) | NVIDIA |\n", | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pick a real contributor like Mrunal Patel |
||
| ) | ||
| body = alert.format_delta(TABLE, after) | ||
| assert "@newbie" in body | ||
| assert "@pimlock" in body | ||
|
|
||
|
|
||
| def test_reports_no_change_when_only_prose_moves() -> None: | ||
| body = alert.format_delta(TABLE, TABLE + "\nSee also CONTRIBUTING.md.\n") | ||
| assert "does not change" in body | ||
|
|
||
|
|
||
| def test_warns_when_the_result_parses_empty() -> None: | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check should fail in this condition |
||
| body = alert.format_delta(TABLE, "# Maintainers\n\n- pimlock\n") | ||
| assert "WARNING" in body | ||
|
|
||
|
|
||
| def test_login_pattern_matches_the_gate() -> None: | ||
| # Each tool parses MAINTAINERS.md on its own. If the patterns drift, this | ||
| # alert reports a delta that differs from what the gate enforces. | ||
| import check_maintainer_approval as gate | ||
|
|
||
| assert alert.MAINTAINER_RE.pattern == gate.MAINTAINER_RE.pattern | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just set it as an environment variable that we use here and the alert script gets for free? Less maintenance and coupling is a goodness.