From 6d8ed2b4fc2aaba8c87f44b1cf4931e22b683583 Mon Sep 17 00:00:00 2001 From: Austin Horstman Date: Fri, 11 Jul 2025 15:20:37 -0500 Subject: [PATCH] ci: tag-maintainer workflow refactor (#7436) Break the workflow into multiple scripts to make it easier to test / maintain. Also fix the remove reviewer process to not review reviews from people that were manually requested. Signed-off-by: Austin Horstman --- .github/workflows/tag-maintainers.yml | 127 +++----------- lib/nix/extract-maintainers.nix | 71 ++++++++ lib/python/extract-maintainers.py | 107 ++++++++++++ lib/python/manage-reviewers.py | 231 ++++++++++++++++++++++++++ 4 files changed, 436 insertions(+), 100 deletions(-) create mode 100644 lib/nix/extract-maintainers.nix create mode 100755 lib/python/extract-maintainers.py create mode 100755 lib/python/manage-reviewers.py diff --git a/.github/workflows/tag-maintainers.yml b/.github/workflows/tag-maintainers.yml index c3b75d2b8..ba399eacb 100644 --- a/.github/workflows/tag-maintainers.yml +++ b/.github/workflows/tag-maintainers.yml @@ -12,7 +12,6 @@ jobs: tag-maintainers: runs-on: ubuntu-latest if: | - github.repository_owner == 'nix-community' && github.event.pull_request.draft == false && github.event.pull_request.state == 'open' steps: @@ -48,113 +47,41 @@ jobs: echo "module_files<> $GITHUB_OUTPUT echo "$CHANGED_FILES" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT + - name: Extract Maintainers + id: extract-maintainers + run: | + echo "Extracting maintainers from changed files..." + MAINTAINERS=$(lib/python/extract-maintainers.py \ + --changed-files "${{ steps.changed-files.outputs.module_files }}" \ + --pr-author "${{ github.event.pull_request.user.login }}") + echo "maintainers=$MAINTAINERS" >> $GITHUB_OUTPUT + echo "Found maintainers: $MAINTAINERS" - name: Manage Reviewers env: GH_TOKEN: ${{ steps.app-token.outputs.token || secrets.GITHUB_TOKEN }} run: | - remove_reviewers() { - local reviewers_to_remove="$1" - local reason="$2" - - if [[ -n "$reviewers_to_remove" ]]; then - for REVIEWER in $reviewers_to_remove; do - echo "Removing review request from $REVIEWER ($reason)" - gh api --method DELETE "/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/requested_reviewers" \ - --input - <<< "{\"reviewers\": [\"$REVIEWER\"]}" - done - fi - } - - # Check if no module files changed - remove all reviewers + # Handle case where no module files changed if [[ '${{ steps.changed-files.outputs.module_files }}' == '' ]]; then - echo "No module files changed, checking for outdated reviewers to remove..." - PENDING_REVIEWERS=$(gh pr view ${{ github.event.pull_request.number }} --json reviewRequests --jq '.reviewRequests[].login') - if [[ -n "$PENDING_REVIEWERS" ]]; then - echo "Found pending reviewers to remove: $PENDING_REVIEWERS" - remove_reviewers "$PENDING_REVIEWERS" "no module files changed" - else - echo "No pending reviewers to remove." - fi + echo "No module files changed, managing reviewers accordingly..." + lib/python/manage-reviewers.py \ + --owner "${{ github.repository_owner }}" \ + --repo "${{ github.event.repository.name }}" \ + --pr-number "${{ github.event.pull_request.number }}" \ + --pr-author "${{ github.event.pull_request.user.login }}" \ + --no-module-files exit 0 fi - # Process module files to find current maintainers - declare -A MAINTAINERS_TO_NOTIFY - PR_AUTHOR="${{ github.event.pull_request.user.login }}" - - while IFS= read -r FILE; do - if [[ -z "$FILE" ]]; then - continue - fi - - echo "Processing file: $FILE" - - MAINTAINERS_JSON=$(nix eval --impure --expr " - let - nixpkgs = import {}; - lib = import ./modules/lib/stdlib-extended.nix nixpkgs.lib; - pkgs = nixpkgs; - config = {}; - module = import ./$FILE { inherit lib pkgs config; }; - in - module.meta.maintainers or [] - " --json 2>/dev/null || echo "[]") - - if [[ "$MAINTAINERS_JSON" == "[]" ]]; then - echo "No maintainers found for $FILE" - continue - fi - - echo "Found maintainers JSON for $FILE: $MAINTAINERS_JSON" - - # Extract GitHub usernames from the maintainers - MAINTAINERS=$(echo "$MAINTAINERS_JSON" | jq -r '.[] | .github // empty' 2>/dev/null || echo "") - - for MAINTAINER in $MAINTAINERS; do - if [[ "$MAINTAINER" != "$PR_AUTHOR" ]]; then - MAINTAINERS_TO_NOTIFY["$MAINTAINER"]=1 - echo "Found maintainer for $FILE: $MAINTAINER" - fi - done - done <<< "${{ steps.changed-files.outputs.module_files }}" - - if [[ ${#MAINTAINERS_TO_NOTIFY[@]} -gt 0 ]]; then - PENDING_REVIEWERS=$(gh pr view ${{ github.event.pull_request.number }} --json reviewRequests --jq '.reviewRequests[].login') - PAST_REVIEWERS=$(gh api "repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/reviews" --jq '.[].user.login') - USERS_TO_EXCLUDE=$(printf "%s\n%s" "$PENDING_REVIEWERS" "$PAST_REVIEWERS" | sort -u) - echo "Complete list of users to exclude:" - echo "$USERS_TO_EXCLUDE" - - # Remove outdated review requests - CURRENT_MAINTAINERS=$(printf "%s\n" "${!MAINTAINERS_TO_NOTIFY[@]}" | sort -u) - OUTDATED_REVIEWERS=$(comm -23 <(echo "$PENDING_REVIEWERS" | sort) <(echo "$CURRENT_MAINTAINERS" | sort)) - remove_reviewers "$OUTDATED_REVIEWERS" "no longer a maintainer of changed files" - - # Check if maintainers are collaborators and not already reviewers - REPO="${{ github.repository }}" - NEW_REVIEWERS=() - for MAINTAINER in "${!MAINTAINERS_TO_NOTIFY[@]}"; do - if echo "$USERS_TO_EXCLUDE" | grep -q -w "$MAINTAINER"; then - echo "$MAINTAINER is already a reviewer, skipping." - continue - fi - - echo "Checking if $MAINTAINER is a collaborator..." - if gh api "/repos/$REPO/collaborators/$MAINTAINER" --silent; then - echo "User $MAINTAINER is a collaborator, adding to new reviewers list" - NEW_REVIEWERS+=("$MAINTAINER") - else - echo "User $MAINTAINER is not a repository collaborator, probably missed the automated invite to the maintainers team, ignoring" - fi - done - - if [[ ${#NEW_REVIEWERS[@]} -gt 0 ]]; then - REVIEWERS_CSV=$(printf "%s," "${NEW_REVIEWERS[@]}") - echo "Requesting reviews from: ${REVIEWERS_CSV%,}" - gh pr edit ${{ github.event.pull_request.number }} --add-reviewer "${REVIEWERS_CSV%,}" - else - echo "No new reviewers to add." - fi + # Handle case where module files changed + MAINTAINERS="${{ steps.extract-maintainers.outputs.maintainers }}" + if [[ -n "$MAINTAINERS" ]]; then + echo "Managing reviewers for maintainers: $MAINTAINERS" + lib/python/manage-reviewers.py \ + --owner "${{ github.repository_owner }}" \ + --repo "${{ github.event.repository.name }}" \ + --pr-number "${{ github.event.pull_request.number }}" \ + --pr-author "${{ github.event.pull_request.user.login }}" \ + --current-maintainers "$MAINTAINERS" else echo "No module maintainers found for the modified files." fi diff --git a/lib/nix/extract-maintainers.nix b/lib/nix/extract-maintainers.nix new file mode 100644 index 000000000..61c83cdca --- /dev/null +++ b/lib/nix/extract-maintainers.nix @@ -0,0 +1,71 @@ +{ + lib ? import ../../modules/lib/stdlib-extended.nix (import { }).lib, + changedFilesJson ? throw "provide either changedFiles or changedFilesJson", + changedFiles ? builtins.fromJSON changedFilesJson, +}: +let + config = { }; + releaseInfo = lib.importJSON ../../release.json; + + extractMaintainersFromFile = + file: + let + isNixFile = lib.hasSuffix ".nix" file; + + moduleResult = + if isNixFile then + let + result = builtins.tryEval ( + let + fileContent = import (../../. + "/${file}"); + + module = + if lib.isFunction fileContent then + # TODO: Find a better way of handling this... + if lib.hasPrefix "docs/" file then + if lib.hasSuffix "home-manager-manual.nix" file then + fileContent { + stdenv = { + mkDerivation = x: x; + }; + inherit lib; + documentation-highlighter = { }; + revision = "unknown"; + home-manager-options = { + home-manager = { }; + nixos = { }; + nix-darwin = { }; + }; + nixos-render-docs = { }; + } + else + fileContent { + inherit lib; + pkgs = null; + inherit (releaseInfo) release isReleaseBranch; + } + else if lib.hasPrefix "lib/" file then + fileContent { inherit lib; } + else + fileContent { + inherit lib config; + pkgs = null; + } + else + fileContent; + in + module.meta.maintainers or [ ] + ); + in + if result.success then result.value else [ ] + else + [ ]; + in + moduleResult; +in +lib.pipe changedFiles [ + (map extractMaintainersFromFile) + lib.concatLists + lib.unique + (map (maintainer: maintainer.github)) +] diff --git a/lib/python/extract-maintainers.py b/lib/python/extract-maintainers.py new file mode 100755 index 000000000..337b39c5d --- /dev/null +++ b/lib/python/extract-maintainers.py @@ -0,0 +1,107 @@ +#!/usr/bin/env python3 +""" +Extract maintainers from changed Home Manager module files. + +This script extracts the maintainer extraction logic from the tag-maintainers workflow +for easier testing and validation. +""" + +import argparse +import json +import logging +import subprocess +import sys +from pathlib import Path + + +class NixEvalError(Exception): + """Custom exception for errors during Nix evaluation.""" + pass + + +def run_nix_eval(nix_file: Path, *args: str) -> str: + """Run a Nix evaluation expression and return the result as a string.""" + command = [ + "nix-instantiate", + "--eval", + "--strict", + "--json", + str(nix_file), + *args, + ] + logging.debug(f"Running command: {' '.join(command)}") + try: + result = subprocess.run( + command, + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + except FileNotFoundError: + logging.error("'nix-instantiate' command not found. Is Nix installed and in your PATH?") + raise NixEvalError("'nix-instantiate' not found") + except subprocess.CalledProcessError as e: + logging.error(f"Nix evaluation failed with exit code {e.returncode}") + logging.error(f"Stderr: {e.stderr.strip()}") + raise NixEvalError("Nix evaluation failed") from e + + +def extract_maintainers(changed_files: list[str], pr_author: str) -> list[str]: + """Extract and filter maintainers from a list of changed module files.""" + if not changed_files: + logging.info("No module files changed; no maintainers to tag.") + return [] + + logging.info("Finding maintainers for changed files...") + nix_file = Path(__file__).parent.parent / "nix" / "extract-maintainers.nix" + changed_files_json = json.dumps(changed_files) + + try: + result_json = run_nix_eval(nix_file, "--argstr", "changedFilesJson", changed_files_json) + maintainers = set(json.loads(result_json)) + except NixEvalError: + # Error is already logged by run_nix_eval + return [] + except json.JSONDecodeError as e: + logging.error(f"Error parsing JSON output from Nix: {e}") + return [] + + filtered_maintainers = sorted(list(maintainers - {pr_author})) + + if not filtered_maintainers: + logging.info("No maintainers found (or only the PR author is a maintainer).") + return [] + + logging.info(f"Found maintainers to notify: {' '.join(filtered_maintainers)}") + return filtered_maintainers + + +def main() -> None: + """Parse arguments and run the maintainer extraction.""" + logging.basicConfig(level=logging.INFO, format="%(message)s", stream=sys.stderr) + + parser = argparse.ArgumentParser( + description="Extract maintainers from changed Home Manager module files." + ) + parser.add_argument( + "--changed-files", + help="Newline-separated list of changed files", + default="", + ) + parser.add_argument( + "--pr-author", + required=True, + help="GitHub username of the PR author", + ) + args = parser.parse_args() + + changed_files = [f.strip() for f in args.changed_files.splitlines() if f.strip()] + + maintainers = extract_maintainers(changed_files, args.pr_author) + + print(" ".join(maintainers)) + + +if __name__ == "__main__": + main() diff --git a/lib/python/manage-reviewers.py b/lib/python/manage-reviewers.py new file mode 100755 index 000000000..ed39f0365 --- /dev/null +++ b/lib/python/manage-reviewers.py @@ -0,0 +1,231 @@ +#!/usr/bin/env python3 +""" +Manage pull request reviewers for Home Manager. + +This script handles the reviewer management logic from the tag-maintainers workflow, +including checking for manually requested reviewers and managing removals. +""" + +import argparse +import json +import logging +import subprocess +import sys +from typing import Final + +# Configure logging to output to stderr +logging.basicConfig( + level=logging.INFO, + format="%(message)s", + stream=sys.stderr, +) + +MANUAL_REVIEW_REQUEST_QUERY: Final[str] = """ +query($owner: String!, $repo: String!, $prNumber: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $prNumber) { + timelineItems(first: 100, itemTypes: [REVIEW_REQUESTED_EVENT]) { + nodes { + ... on ReviewRequestedEvent { + actor { + __typename + login + } + requestedReviewer { + ... on User { login } + ... on Bot { login } + } + } + } + } + } + } +} +""" + + +class GHError(Exception): + """Custom exception for errors related to 'gh' CLI commands.""" + pass + + +def run_gh_command(args: list[str], input_data: str | None = None) -> str: + """Runs a GitHub CLI command and returns its stdout.""" + command = ["gh"] + args + try: + result = subprocess.run( + command, + input=input_data, + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + except subprocess.CalledProcessError as e: + logging.error("Error running command: %s", " ".join(command)) + logging.error("Stderr: %s", e.stderr.strip()) + raise GHError(f"Failed to execute gh command: {e}") from e + + +def get_manually_requested_reviewers( + owner: str, repo: str, pr_number: int +) -> set[str]: + """Fetches a set of reviewers who were manually requested by a human.""" + try: + result = run_gh_command([ + "api", "graphql", + "-f", f"query={MANUAL_REVIEW_REQUEST_QUERY}", + "-F", f"owner={owner}", + "-F", f"repo={repo}", + "-F", f"prNumber={pr_number}", + ]) + data = json.loads(result) + nodes = data.get("data", {}).get("repository", {}).get("pullRequest", {}).get("timelineItems", {}).get("nodes", []) + + manually_requested = { + node["requestedReviewer"]["login"] + for node in nodes + if node and node.get("actor", {}).get("__typename") == "User" and node.get("requestedReviewer") + } + return manually_requested + except (GHError, json.JSONDecodeError, KeyError) as e: + logging.error("Could not determine manually requested reviewers: %s", e) + return set() + + +def get_users_from_gh(args: list[str], error_message: str) -> set[str]: + """A generic helper to get a set of users from a 'gh' command.""" + try: + result = run_gh_command(args) + return {user.strip() for user in result.split("\n") if user.strip()} + except GHError as e: + logging.error("%s: %s", error_message, e) + return set() + + +def get_pending_reviewers(pr_number: int) -> set[str]: + """Gets the set of currently pending reviewers for a PR.""" + return get_users_from_gh( + ["pr", "view", str(pr_number), "--json", "reviewRequests", "--jq", ".reviewRequests[].login"], + "Error getting pending reviewers", + ) + + +def get_past_reviewers(owner: str, repo: str, pr_number: int) -> set[str]: + """Gets the set of users who have already reviewed the PR.""" + return get_users_from_gh( + ["api", f"repos/{owner}/{repo}/pulls/{pr_number}/reviews", "--jq", ".[].user.login"], + "Error getting past reviewers", + ) + + +def is_collaborator(owner: str, repo: str, username: str) -> bool: + """Checks if a user is a collaborator on the repository.""" + try: + # The `--silent` flag makes `gh` exit with 0 on success (2xx) and 1 on error (e.g., 404) + run_gh_command(["api", f"repos/{owner}/{repo}/collaborators/{username}", "--silent"]) + return True + except GHError: + # A non-zero exit code (GHError) implies the user is not a collaborator (404) or another issue. + return False + + +def update_reviewers( + pr_number: int, + reviewers_to_add: set[str] | None = None, + reviewers_to_remove: set[str] | None = None, + owner: str | None = None, + repo: str | None = None, +) -> None: + """Adds or removes reviewers from a PR in a single operation per action.""" + if reviewers_to_add: + logging.info("Requesting reviews from: %s", ", ".join(reviewers_to_add)) + try: + run_gh_command([ + "pr", "edit", str(pr_number), + "--add-reviewer", ",".join(reviewers_to_add) + ]) + except GHError as e: + logging.error("Failed to add reviewers: %s", e) + + if reviewers_to_remove and owner and repo: + logging.info("Removing review requests from: %s", ", ".join(reviewers_to_remove)) + payload = json.dumps({"reviewers": list(reviewers_to_remove)}) + try: + run_gh_command( + [ + "api", "--method", "DELETE", + f"repos/{owner}/{repo}/pulls/{pr_number}/requested_reviewers", + "--input", "-", + ], + input_data=payload, + ) + except GHError as e: + logging.error("Failed to remove reviewers: %s", e) + + +def main() -> None: + """Main function to handle command-line arguments and manage reviewers.""" + parser = argparse.ArgumentParser(description="Manage pull request reviewers for Home Manager.") + parser.add_argument("--owner", required=True, help="Repository owner.") + parser.add_argument("--repo", required=True, help="Repository name.") + parser.add_argument("--pr-number", type=int, required=True, help="Pull request number.") + parser.add_argument("--pr-author", required=True, help="PR author's username.") + parser.add_argument("--current-maintainers", default="", help="Space-separated list of current maintainers.") + parser.add_argument("--no-module-files", action="store_true", help="Flag if no module files were changed.") + args = parser.parse_args() + + # --- 1. Fetch current state from GitHub --- + maintainers: set[str] = set(args.current_maintainers.split()) + pending_reviewers = get_pending_reviewers(args.pr_number) + past_reviewers = get_past_reviewers(args.owner, args.repo, args.pr_number) + manually_requested = get_manually_requested_reviewers(args.owner, args.repo, args.pr_number) + + logging.info("Current Maintainers: %s", ' '.join(maintainers) or "None") + logging.info("Pending Reviewers: %s", ' '.join(pending_reviewers) or "None") + logging.info("Past Reviewers: %s", ' '.join(past_reviewers) or "None") + logging.info("Manually Requested: %s", ' '.join(manually_requested) or "None") + + # --- 2. Determine reviewers to remove --- + reviewers_to_remove: set[str] = set() + if args.no_module_files: + reviewers_to_remove = pending_reviewers - manually_requested + logging.info("No module files changed. Removing bot-requested reviewers.") + else: + outdated_reviewers = pending_reviewers - maintainers + reviewers_to_remove = outdated_reviewers - manually_requested + logging.info("Removing outdated bot-requested reviewers.") + + if reviewers_to_remove: + update_reviewers( + args.pr_number, + owner=args.owner, + repo=args.repo, + reviewers_to_remove=reviewers_to_remove + ) + else: + logging.info("No reviewers to remove.") + + # --- 3. Determine new reviewers to add --- + reviewers_to_add: set[str] = set() + if not args.no_module_files and maintainers: + users_to_exclude = {args.pr_author} | past_reviewers | pending_reviewers + potential_reviewers = maintainers - users_to_exclude + + reviewers_to_add = { + user for user in potential_reviewers if is_collaborator(args.owner, args.repo, user) + } + + non_collaborators = potential_reviewers - reviewers_to_add + if non_collaborators: + logging.warning("Ignoring non-collaborators: %s", ", ".join(non_collaborators)) + + + if reviewers_to_add: + update_reviewers(args.pr_number, reviewers_to_add=reviewers_to_add) + else: + logging.info("No new reviewers to add.") + + +if __name__ == "__main__": + main()