diff --git a/.github/workflows/tag-maintainers.yml b/.github/workflows/tag-maintainers.yml index ba399eacb..e25b700ef 100644 --- a/.github/workflows/tag-maintainers.yml +++ b/.github/workflows/tag-maintainers.yml @@ -59,29 +59,19 @@ jobs: - name: Manage Reviewers env: GH_TOKEN: ${{ steps.app-token.outputs.token || secrets.GITHUB_TOKEN }} + OWNER: ${{ github.repository_owner }} + REPO: ${{ github.event.repository.name }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + MAINTAINERS: ${{ steps.extract-maintainers.outputs.maintainers }} + CHANGED_FILES: ${{ steps.changed-files.outputs.changed_files }} + BOT_NAME: ${{ steps.app-token.outputs.app-slug || 'github-actions' }} run: | - # Handle case where no module files changed - if [[ '${{ steps.changed-files.outputs.module_files }}' == '' ]]; then - 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 - - # 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 + ./ci/tag-maintainers/manage-reviewers.py \ + --owner "$OWNER" \ + --repo "$REPO" \ + --pr-number "$PR_NUMBER" \ + --pr-author "$PR_AUTHOR" \ + --current-maintainers "$MAINTAINERS" \ + --changed-files "$CHANGED_FILES" \ + --bot-user-name "$BOT_NAME" diff --git a/lib/python/manage-reviewers.py b/lib/python/manage-reviewers.py index ed39f0365..de55dd971 100755 --- a/lib/python/manage-reviewers.py +++ b/lib/python/manage-reviewers.py @@ -24,7 +24,7 @@ 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]) { + timelineItems(last: 250, itemTypes: [REVIEW_REQUESTED_EVENT]) { nodes { ... on ReviewRequestedEvent { actor { @@ -49,8 +49,12 @@ class GHError(Exception): pass -def run_gh_command(args: list[str], input_data: str | None = None) -> str: - """Runs a GitHub CLI command and returns its stdout.""" +def run_gh_command( + args: list[str], + input_data: str | None = None, + check: bool = True, +) -> subprocess.CompletedProcess: + """Runs a GitHub CLI command and returns the CompletedProcess object.""" command = ["gh"] + args try: result = subprocess.run( @@ -58,9 +62,9 @@ def run_gh_command(args: list[str], input_data: str | None = None) -> str: input=input_data, capture_output=True, text=True, - check=True, + check=check, ) - return result.stdout.strip() + return result except subprocess.CalledProcessError as e: logging.error("Error running command: %s", " ".join(command)) logging.error("Stderr: %s", e.stderr.strip()) @@ -68,9 +72,9 @@ def run_gh_command(args: list[str], input_data: str | None = None) -> str: def get_manually_requested_reviewers( - owner: str, repo: str, pr_number: int + owner: str, repo: str, pr_number: int, bot_user_name: str ) -> set[str]: - """Fetches a set of reviewers who were manually requested by a human.""" + """Fetches a set of reviewers who were manually requested by someone other than the bot.""" try: result = run_gh_command([ "api", "graphql", @@ -79,13 +83,13 @@ def get_manually_requested_reviewers( "-F", f"repo={repo}", "-F", f"prNumber={pr_number}", ]) - data = json.loads(result) + data = json.loads(result.stdout) 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") + if node and node.get("requestedReviewer") and node.get("actor", {}).get("login") != bot_user_name } return manually_requested except (GHError, json.JSONDecodeError, KeyError) as e: @@ -97,7 +101,7 @@ 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()} + return {user.strip() for user in result.stdout.split("\n") if user.strip()} except GHError as e: logging.error("%s: %s", error_message, e) return set() @@ -120,14 +124,31 @@ def get_past_reviewers(owner: str, repo: str, pr_number: int) -> set[str]: 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"]) + """ + Checks if a user is a collaborator on the repository. + Handles 404 as a non-collaborator, while other errors are raised. + """ + result = run_gh_command( + ["api", f"repos/{owner}/{repo}/collaborators/{username}"], + check=False + ) + + if result.returncode == 0: return True - except GHError: - # A non-zero exit code (GHError) implies the user is not a collaborator (404) or another issue. + + if "HTTP 404" in result.stderr: + logging.error( + "'%s' is not a collaborator in this repository.", username + ) return False + else: + logging.error( + "Unexpected error checking collaborator status for '%s'.", username + ) + logging.error("Stderr: %s", result.stderr.strip()) + raise GHError( + f"Unexpected API error for user '{username}': {result.stderr.strip()}" + ) def update_reviewers( @@ -172,14 +193,17 @@ def main() -> None: 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.") + parser.add_argument("--changed-files", default="", help="Newline-separated list of changed files.") + parser.add_argument("--bot-user-name", default="", help="Bot user name to distinguish manual vs automated review requests.") args = parser.parse_args() + no_changed_files = not args.changed_files.strip() + # --- 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) + manually_requested = get_manually_requested_reviewers(args.owner, args.repo, args.pr_number, args.bot_user_name) logging.info("Current Maintainers: %s", ' '.join(maintainers) or "None") logging.info("Pending Reviewers: %s", ' '.join(pending_reviewers) or "None") @@ -188,7 +212,7 @@ def main() -> None: # --- 2. Determine reviewers to remove --- reviewers_to_remove: set[str] = set() - if args.no_module_files: + if no_changed_files: reviewers_to_remove = pending_reviewers - manually_requested logging.info("No module files changed. Removing bot-requested reviewers.") else: @@ -208,7 +232,7 @@ def main() -> None: # --- 3. Determine new reviewers to add --- reviewers_to_add: set[str] = set() - if not args.no_module_files and maintainers: + if not no_changed_files and maintainers: users_to_exclude = {args.pr_author} | past_reviewers | pending_reviewers potential_reviewers = maintainers - users_to_exclude @@ -220,7 +244,6 @@ def main() -> None: 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: