mirror of
https://github.com/HDFGroup/hdf5.git
synced 2026-09-25 04:09:44 +03:00
CI: fix reviewer workflows for already-reviewed and draft PRs (#6453)
* CI: skip reviewer removal when they have already submitted a review
GitHub's API rejects removeRequestedReviewers for users who have already
reviewed; detect that case upfront via listReviews and report it clearly
instead of surfacing a cryptic API error.
* CI: defer reviewer assignment for draft PRs until ready for review
Add ready_for_review to the pull_request_target trigger so the workflow
fires when a draft is promoted. In the script, skip requestReviewers
(and the CODEOWNERS auto-assignment cleanup second pass) while the PR is
a draft; still assign the PR author and strip any reviewers GitHub
auto-assigned. When the PR is marked ready, treat it like a fresh open
and run the full load-balanced reviewer selection.
* CI: fix synchronize race — treat first synchronize as new PR when opened was cancelled
When a fork PR is created, GitHub fires both opened and synchronize.
With cancel-in-progress: true the synchronize run often wins, and the
opened run (which clears CODEOWNERS auto-assignments) is cancelled before
it completes. Detect this: if synchronize fires but no checklist comment
exists yet, run the full new-PR path (enforceSelection + load-balanced
reviewer assignment) instead of silently carrying forward all auto-assigned
reviewers.
* CI: enforce reviewer selection before posting @mentions
On synchronize, run enforceSelection against the ideal load-balanced
pick before building the checklist body so the @mentions are never
sent until the reviewer list is actually correct. Re-fetch the PR
after cleanup so confirmedRequested reflects reality, not the pre-
cleanup snapshot.
For workflow_run (review submitted), reviewer assignment is intentionally
left unchanged but @mentions are filtered to the ideal selection so
CODEOWNERS extras don't generate spurious notifications on comment edits.
* CI: preserve manually added reviewers on synchronize
Reverting the enforceSelection call on the non-first-run synchronize
path. There is no API way to distinguish CODEOWNERS auto-assignments
from manually added reviewers, so enforcing the load-balanced selection
on every synchronize would silently remove intentional additions.
The initial cleanup (opened or first-synchronize via openedWasSkipped)
already produces a correct reviewer list; subsequent synchronize and
workflow_run events carry it forward unchanged.
* CI: pin actions/github-script to commit hash in remove-reviewer.yml
zizmor requires actions to be pinned to a commit hash rather than a tag.
Use the same pinned hash (ed597411d8f924073f98dfc5c65a23a2325f34cd, v8.0.0)
already used in review-checklist.yml.
* CI: refactor review-checklist.js for readability
Extract the reviewer coordination logic out of the monolithic run()
function into named module-level helpers:
checklistExists() — single responsibility: does a checklist
comment already exist on the PR?
removeUnselected() — remove CODEOWNERS auto-assignments not in
the load-balanced selection set
requestReviewers() — request each reviewer individually so one
bad login cannot block the rest
removeUnselectedAfterDelay() — 15-second wait + re-fetch + re-enforce
for GitHub's async auto-assignment race
coordinateReviewers() — top-level dispatcher; the four event paths
(read-only, synchronize-normal, new-PR/draft,
new-PR/non-draft) are now explicit branches
with labelled comments instead of nested ifs
run() is now a straight pipeline of 8 numbered steps with no nested
async functions. Behaviour is unchanged.
* CI: extract convertGlobToRegex and use github.paginate consistently
Extract glob-to-regex conversion into a standalone helper and replace
manual pagination loops for listFiles and listReviews with github.paginate,
matching the existing style used for listComments and pulls.list.
* CI: restrict remove-reviewer workflow to main repo only
Adds a github.repository guard so the job does not run in forks,
preventing unintended resource consumption and command execution
against fork PRs.
* CI: restrict test-maven-packages workflow to main repo only
Adds github.repository guards to all three jobs so the workflow
does not evaluate in forks, preventing spurious "workflow file issue"
failures on push events in forked repositories.
* Revert "CI: restrict test-maven-packages workflow to main repo only"
This reverts commit cc1208226d.
This commit is contained in:
Scot Breitenfeld
+232
-203
@@ -2,11 +2,25 @@
|
||||
|
||||
const MARKER = '<!-- hdf5-review-checklist-v1 -->';
|
||||
|
||||
// ── Pure helpers ──────────────────────────────────────────────────────────────
|
||||
|
||||
function labelFromPattern(pattern) {
|
||||
// /fortran/ → "fortran", /.github/.well-known → ".github/.well-known"
|
||||
return pattern.replace(/^\//, '').replace(/\/$/, '') || pattern;
|
||||
}
|
||||
|
||||
// Converts a CODEOWNERS glob pattern to a RegExp.
|
||||
// Process ** before * so single-star replacement cannot corrupt double-star tokens.
|
||||
function convertGlobToRegex(p, anchored) {
|
||||
let escaped = p.replace(/[.+^${}()|[\]\\]/g, '\\$&');
|
||||
escaped = escaped.replace(/\/\*\*\//g, '/(?:.+/)?'); // /**/ → zero or more subdirectories
|
||||
escaped = escaped.replace(/^\*\*\//, '(?:.+/)?'); // **/ at start → optional leading dirs
|
||||
escaped = escaped.replace(/\/\*\*$/, '(?:/.+)?'); // /** at end → optional trailing path
|
||||
escaped = escaped.replace(/\*\*/g, '.*'); // bare ** → anything
|
||||
escaped = escaped.replace(/\*/g, '[^/]*'); // * → single path component
|
||||
return new RegExp((anchored ? '^' : '(^|/)') + escaped + '($|/)');
|
||||
}
|
||||
|
||||
// Returns true if `file` (repo-relative, no leading slash) matches
|
||||
// a CODEOWNERS-style gitignore pattern.
|
||||
function matchesPattern(file, pattern) {
|
||||
@@ -22,23 +36,8 @@ function matchesPattern(file, pattern) {
|
||||
: (file === p.slice(0, -1) || file.startsWith(p) || file.includes('/' + p));
|
||||
}
|
||||
|
||||
// Glob pattern: convert * and ** to regex equivalents.
|
||||
// Process ** before * so the single-star replacement cannot corrupt double-star tokens.
|
||||
if (p.includes('*')) {
|
||||
// Escape regex metacharacters, leaving * intact for the steps below
|
||||
let escaped = p.replace(/[.+^${}()|[\]\\]/g, '\\$&');
|
||||
// /**/ → zero or more path components (zero depth = just a single slash separator)
|
||||
escaped = escaped.replace(/\/\*\*\//g, '/(?:.+/)?');
|
||||
// **/ at start → any leading directories (including none)
|
||||
escaped = escaped.replace(/^\*\*\//, '(?:.+/)?');
|
||||
// /** at end → any trailing path (including none)
|
||||
escaped = escaped.replace(/\/\*\*$/, '(?:/.+)?');
|
||||
// bare ** → anything (fallback for patterns like **)
|
||||
escaped = escaped.replace(/\*\*/g, '.*');
|
||||
// single * → within one path component only
|
||||
escaped = escaped.replace(/\*/g, '[^/]*');
|
||||
const re = new RegExp((anchored ? '^' : '(^|/)') + escaped + '($|/)');
|
||||
return re.test(file);
|
||||
return convertGlobToRegex(p, anchored).test(file);
|
||||
}
|
||||
|
||||
// Plain path: exact match or directory prefix
|
||||
@@ -113,9 +112,9 @@ function chooseReviewers(touchedAreas, {
|
||||
continue;
|
||||
}
|
||||
|
||||
const threshold = (AREA_THRESHOLDS && AREA_THRESHOLDS[area.label]) ?? LINE_THRESHOLD;
|
||||
const threshold = (AREA_THRESHOLDS && AREA_THRESHOLDS[area.label]) ?? LINE_THRESHOLD;
|
||||
const touchesPublicHeader = area.files.some(f => PUBLIC_HEADER.test(f.filename));
|
||||
const isComplex = area.linesChanged >= threshold || touchesPublicHeader;
|
||||
const isComplex = area.linesChanged >= threshold || touchesPublicHeader;
|
||||
|
||||
if (isComplex) {
|
||||
const pick = area.owners.find(u => u !== prAuthor) ?? null;
|
||||
@@ -196,8 +195,185 @@ function buildBody(touchedAreas, approvedUsers, confirmedRequested) {
|
||||
return parts.join('\n');
|
||||
}
|
||||
|
||||
// ── GitHub API helpers ────────────────────────────────────────────────────────
|
||||
|
||||
// Returns true if the PR already has a checklist comment (MARKER present).
|
||||
async function checklistExists(github, { owner, repo, pr_number }) {
|
||||
const comments = await github.paginate(github.rest.issues.listComments, {
|
||||
owner, repo, issue_number: pr_number, per_page: 100,
|
||||
});
|
||||
return comments.some(c => c.body.includes(MARKER));
|
||||
}
|
||||
|
||||
// Removes CODEOWNERS-auto-assigned reviewers whose login is NOT in keepSet.
|
||||
// Non-owner reviewers are never touched.
|
||||
async function removeUnselected(github, core, { owner, repo, pr_number }, allCodeOwners, currentRequested, keepSet) {
|
||||
for (const reviewer of currentRequested) {
|
||||
if (allCodeOwners.has(reviewer) && !keepSet.has(reviewer)) {
|
||||
try {
|
||||
await github.rest.pulls.removeRequestedReviewers({
|
||||
owner, repo, pull_number: pr_number, reviewers: [reviewer],
|
||||
});
|
||||
core.info(`Removed auto-assigned reviewer ${reviewer} (not in load-balanced selection)`);
|
||||
} catch (e) {
|
||||
core.warning(`Could not remove reviewer ${reviewer}: ${e.message}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Requests each reviewer individually (so one bad login can't block the rest).
|
||||
// Returns the Set of logins that were successfully requested.
|
||||
async function requestReviewers(github, core, { owner, repo, pr_number }, selected) {
|
||||
const confirmed = new Set();
|
||||
for (const reviewer of selected) {
|
||||
try {
|
||||
await github.rest.pulls.requestReviewers({
|
||||
owner, repo, pull_number: pr_number, reviewers: [reviewer],
|
||||
});
|
||||
confirmed.add(reviewer);
|
||||
} catch (e) {
|
||||
core.warning(`Could not request reviewer ${reviewer}: ${e.message}`);
|
||||
}
|
||||
}
|
||||
return confirmed;
|
||||
}
|
||||
|
||||
// Waits for GitHub's async CODEOWNERS auto-assignment to fire, then re-fetches
|
||||
// the PR and removes any code owners that snuck in outside the selection set.
|
||||
async function removeUnselectedAfterDelay(github, core, { owner, repo, pr_number }, allCodeOwners, keepSet) {
|
||||
await new Promise(resolve => setTimeout(resolve, 15000));
|
||||
let retryPR;
|
||||
try {
|
||||
({ data: retryPR } = await github.rest.pulls.get({ owner, repo, pull_number: pr_number }));
|
||||
} catch (e) {
|
||||
core.warning(`Could not re-fetch PR for reviewer cleanup retry: ${e.message}`);
|
||||
return;
|
||||
}
|
||||
const retryRequested = new Set(retryPR.requested_reviewers.map(r => r.login).filter(Boolean));
|
||||
await removeUnselected(github, core, { owner, repo, pr_number }, allCodeOwners, retryRequested, keepSet);
|
||||
}
|
||||
|
||||
// ── Reviewer coordination ─────────────────────────────────────────────────────
|
||||
//
|
||||
// Determines who should be in confirmedRequested (the checklist display set).
|
||||
// Returns a Set<login>. There are four distinct paths:
|
||||
//
|
||||
// read-only (workflow_run, pull_request_review)
|
||||
// → reflect whoever GitHub currently has as requested reviewers
|
||||
//
|
||||
// synchronize (normal push to open PR, checklist already exists)
|
||||
// → preserve the existing assignment unchanged
|
||||
//
|
||||
// new-PR (opened / reopened / ready_for_review, or first-synchronize
|
||||
// race where opened was cancelled before the checklist existed)
|
||||
// draft → clear auto-assignments; defer requests until ready for review
|
||||
// non-draft → full flow: clear → request → wait 15 s → re-clear
|
||||
//
|
||||
async function coordinateReviewers(github, context, core, {
|
||||
owner, repo, pr_number, prData, allCodeOwners, touchedAreas, reviewerLoad,
|
||||
LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER,
|
||||
}) {
|
||||
const existingRequested = new Set(prData.requested_reviewers.map(r => r.login).filter(Boolean));
|
||||
const pr = { owner, repo, pr_number };
|
||||
|
||||
// ── read-only events ─────────────────────────────────────────────────────
|
||||
if (context.eventName === 'pull_request_review' || context.eventName === 'workflow_run') {
|
||||
core.info('Read-only event — reflecting current reviewer assignments');
|
||||
return new Set(existingRequested);
|
||||
}
|
||||
|
||||
const prAuthor = prData.user.login;
|
||||
const isDraft = prData.draft === true;
|
||||
const action = context.payload.action;
|
||||
|
||||
// Assign the PR to its author when they are a code owner.
|
||||
if (allCodeOwners.has(prAuthor)) {
|
||||
try {
|
||||
await github.rest.issues.addAssignees({
|
||||
owner, repo, issue_number: pr_number, assignees: [prAuthor],
|
||||
});
|
||||
core.info(`Assigned PR to author ${prAuthor} (is a code owner)`);
|
||||
} catch (e) {
|
||||
core.warning(`Could not assign PR to author: ${e.message}`);
|
||||
}
|
||||
} else {
|
||||
core.info(`Author ${prAuthor} is not a code owner — skipping assignee`);
|
||||
}
|
||||
|
||||
// ── synchronize race detection ───────────────────────────────────────────
|
||||
// When a fork PR is created, GitHub fires both `opened` and `synchronize`.
|
||||
// With cancel-in-progress: true the synchronize run can win and the opened
|
||||
// cleanup is cancelled — leaving all CODEOWNERS auto-assignments intact.
|
||||
// Detect this by checking whether a checklist comment exists yet; if not,
|
||||
// treat this synchronize as a new-PR event.
|
||||
const isFirstSyncRace = action === 'synchronize' &&
|
||||
!(await checklistExists(github, pr));
|
||||
|
||||
const isNewPR = ['opened', 'reopened', 'ready_for_review'].includes(action) ||
|
||||
isFirstSyncRace;
|
||||
|
||||
// ── synchronize (normal) ─────────────────────────────────────────────────
|
||||
if (!isNewPR) {
|
||||
core.info('synchronize — preserving existing reviewer assignments');
|
||||
return new Set(existingRequested);
|
||||
}
|
||||
|
||||
// ── new-PR: load-balanced selection ──────────────────────────────────────
|
||||
const { selected, log } = chooseReviewers(touchedAreas, {
|
||||
prAuthor,
|
||||
existingRequested: new Set(), // ignore existing — this is a fresh assignment
|
||||
reviewerLoad,
|
||||
LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER,
|
||||
});
|
||||
for (const msg of log) core.info(msg);
|
||||
|
||||
if (isDraft) {
|
||||
// Draft: remove any auto-assignments but hold reviewer requests until
|
||||
// the PR is marked ready for review.
|
||||
await removeUnselected(github, core, pr, allCodeOwners, existingRequested, new Set());
|
||||
core.info('Draft PR — reviewer assignment deferred until ready for review');
|
||||
return new Set();
|
||||
}
|
||||
|
||||
// Non-draft: enforce selection, request reviewers, then re-enforce after
|
||||
// GitHub's async CODEOWNERS auto-assignment fires.
|
||||
await removeUnselected(github, core, pr, allCodeOwners, existingRequested, selected);
|
||||
const confirmed = await requestReviewers(github, core, pr, selected);
|
||||
await removeUnselectedAfterDelay(github, core, pr, allCodeOwners, selected);
|
||||
return confirmed;
|
||||
}
|
||||
|
||||
// ── Entry point ───────────────────────────────────────────────────────────────
|
||||
|
||||
module.exports = async function run({ github, context, core }) {
|
||||
const { owner, repo } = context.repo;
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// Configuration
|
||||
//
|
||||
// LINE_THRESHOLD: lines changed within a single area at or above
|
||||
// which the change is considered complex → first (senior) owner
|
||||
// in CODEOWNERS is always assigned.
|
||||
//
|
||||
// PUBLIC_HEADER: files matching this pattern are always treated as
|
||||
// complex regardless of line count — any change to the public or
|
||||
// developer API surface warrants the senior owner.
|
||||
//
|
||||
// Covers: hdf5.h (umbrella), H5*public.h / H5*develop.h (per-module),
|
||||
// VFD driver headers included by hdf5.h, and VOL connector headers.
|
||||
//
|
||||
// NOTE: Team owners (@org/team) in CODEOWNERS are not supported.
|
||||
// Only individual GitHub logins are handled. If teams are added,
|
||||
// extend parsing and reviewer requests to use team_reviewers.
|
||||
// ----------------------------------------------------------------
|
||||
const LINE_THRESHOLD = 300;
|
||||
const AREA_THRESHOLDS = { 'test': 500 }; // test files are verbose; raise bar for senior
|
||||
const PUBLIC_HEADER = /(?:^|\/)hdf5\.h$|public\.h$|develop\.h$|H5FD(?:core|direct|family|hdfs|ioc|log|mirror|mpio?|multi|onion|ros3|sec2|splitter|stdio|subfiling|windows)\.h$|H5VL(?:connector|connector_passthru|native|passthru)\.h$/;
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 1. Resolve the PR number from the triggering event.
|
||||
// ----------------------------------------------------------------
|
||||
let pr_number;
|
||||
|
||||
if (context.eventName === 'workflow_run') {
|
||||
@@ -221,31 +397,7 @@ module.exports = async function run({ github, context, core }) {
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// Configuration
|
||||
//
|
||||
// LINE_THRESHOLD: lines changed within a single area at or above
|
||||
// which the change is considered complex → first (senior) owner
|
||||
// in CODEOWNERS is always assigned.
|
||||
//
|
||||
// PUBLIC_HEADER: files matching this pattern are always treated as
|
||||
// complex regardless of line count — any change to the public or
|
||||
// developer API surface warrants the senior owner.
|
||||
//
|
||||
// Covers: hdf5.h (umbrella), H5*public.h / H5*develop.h (per-module),
|
||||
// VFD driver headers included by hdf5.h, and VOL connector headers.
|
||||
//
|
||||
// NOTE: Team owners (@org/team) in CODEOWNERS are not supported.
|
||||
// Only individual GitHub logins are handled. If teams are added,
|
||||
// extend parsing and reviewer requests to use team_reviewers.
|
||||
// ----------------------------------------------------------------
|
||||
const LINE_THRESHOLD = 300; // default for all areas
|
||||
const AREA_THRESHOLDS = { // per-area overrides
|
||||
'test': 500, // test files are verbose; raise bar for senior
|
||||
};
|
||||
const PUBLIC_HEADER = /(?:^|\/)hdf5\.h$|public\.h$|develop\.h$|H5FD(?:core|direct|family|hdfs|ioc|log|mirror|mpio?|multi|onion|ros3|sec2|splitter|stdio|subfiling|windows)\.h$|H5VL(?:connector|connector_passthru|native|passthru)\.h$/;
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 1. Parse CODEOWNERS into a list of { pattern, label, owners }
|
||||
// 2. Parse CODEOWNERS into { pattern, label, owners }[].
|
||||
// ----------------------------------------------------------------
|
||||
let coText;
|
||||
try {
|
||||
@@ -266,9 +418,7 @@ module.exports = async function run({ github, context, core }) {
|
||||
|
||||
const tokens = line.split(/\s+/);
|
||||
const pattern = tokens[0];
|
||||
const owners = tokens.slice(1)
|
||||
.filter(t => t.startsWith('@'))
|
||||
.map(t => t.slice(1));
|
||||
const owners = tokens.slice(1).filter(t => t.startsWith('@')).map(t => t.slice(1));
|
||||
|
||||
owners.forEach(o => allCodeOwners.add(o));
|
||||
if (pattern === '*') continue;
|
||||
@@ -283,36 +433,26 @@ module.exports = async function run({ github, context, core }) {
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 2. Collect all changed files with line counts.
|
||||
// 3. Collect changed files with per-file line counts.
|
||||
// ----------------------------------------------------------------
|
||||
const changedFileData = [];
|
||||
let changedFileData;
|
||||
try {
|
||||
for (let page = 1; ; page++) {
|
||||
const { data } = await github.rest.pulls.listFiles({
|
||||
owner, repo, pull_number: pr_number, per_page: 100, page,
|
||||
});
|
||||
changedFileData.push(...data);
|
||||
if (data.length < 100) break;
|
||||
}
|
||||
changedFileData = await github.paginate(github.rest.pulls.listFiles, {
|
||||
owner, repo, pull_number: pr_number, per_page: 100,
|
||||
});
|
||||
} catch (error) {
|
||||
core.setFailed(`Failed to list PR files: ${error.message}`);
|
||||
return;
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 3. Attribute files to areas (each file → one area, most-precedent).
|
||||
// Derive per-area line totals and file lists from the same map so
|
||||
// linesChanged and touchesPublicHeader always agree.
|
||||
// 4. Attribute files to areas; derive per-area line totals.
|
||||
// ----------------------------------------------------------------
|
||||
const filesByArea = attributeFiles(changedFileData, areas);
|
||||
const touchedAreas = areas
|
||||
.map(area => {
|
||||
const files = filesByArea.get(area.pattern) || [];
|
||||
return {
|
||||
...area,
|
||||
files,
|
||||
linesChanged: files.reduce((sum, f) => sum + f.changes, 0),
|
||||
};
|
||||
return { ...area, files, linesChanged: files.reduce((sum, f) => sum + f.changes, 0) };
|
||||
})
|
||||
.filter(area => area.linesChanged > 0);
|
||||
|
||||
@@ -337,165 +477,58 @@ module.exports = async function run({ github, context, core }) {
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 4. Determine current approvals.
|
||||
// 5. Fetch reviews and current PR state.
|
||||
// ----------------------------------------------------------------
|
||||
const allReviews = [];
|
||||
let allReviews = [];
|
||||
try {
|
||||
for (let page = 1; ; page++) {
|
||||
const { data } = await github.rest.pulls.listReviews({
|
||||
owner, repo, pull_number: pr_number, per_page: 100, page,
|
||||
});
|
||||
allReviews.push(...data);
|
||||
if (data.length < 100) break;
|
||||
}
|
||||
allReviews = await github.paginate(github.rest.pulls.listReviews, {
|
||||
owner, repo, pull_number: pr_number, per_page: 100,
|
||||
});
|
||||
} catch (error) {
|
||||
core.warning(`Failed to fetch reviews; approval state may be stale: ${error.message}`);
|
||||
}
|
||||
const approvedUsers = computeApprovals(allReviews);
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 5. Fetch current PR state (requested reviewers).
|
||||
// ----------------------------------------------------------------
|
||||
let prData;
|
||||
try {
|
||||
({ data: prData } = await github.rest.pulls.get({
|
||||
owner, repo, pull_number: pr_number,
|
||||
}));
|
||||
({ data: prData } = await github.rest.pulls.get({ owner, repo, pull_number: pr_number }));
|
||||
} catch (error) {
|
||||
core.setFailed(`Failed to fetch PR data: ${error.message}`);
|
||||
return;
|
||||
}
|
||||
const existingRequested = new Set(
|
||||
prData.requested_reviewers.map(r => r.login).filter(Boolean)
|
||||
);
|
||||
|
||||
// confirmedRequested tracks who is actually assigned for checklist display.
|
||||
// Starts empty — populated below only with the load-balanced selection so
|
||||
// the checklist never shows owners that were not chosen by the script.
|
||||
let confirmedRequested = new Set();
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 6. Auto-assign reviewers (pull_request events only, not reviews).
|
||||
// 6. Build reviewer load map (one paginated list call instead of N
|
||||
// Search API calls — the Search API caps at 30 req/min).
|
||||
// ----------------------------------------------------------------
|
||||
if (context.eventName !== 'pull_request_review' && context.eventName !== 'workflow_run') {
|
||||
const prAuthor = prData.user.login;
|
||||
|
||||
// Assign the PR author only if they are a code owner.
|
||||
if (allCodeOwners.has(prAuthor)) {
|
||||
try {
|
||||
await github.rest.issues.addAssignees({
|
||||
owner, repo, issue_number: pr_number, assignees: [prAuthor],
|
||||
});
|
||||
core.info(`Assigned PR to author ${prAuthor} (is a code owner)`);
|
||||
} catch (e) {
|
||||
core.warning(`Could not assign PR to author: ${e.message}`);
|
||||
}
|
||||
} else {
|
||||
core.info(`Author ${prAuthor} is not a code owner — skipping assignee`);
|
||||
}
|
||||
|
||||
// One paginated pulls.list call instead of N Search API calls — the Search API
|
||||
// allows only 30 req/min; with several candidates per area that limit is easy to hit.
|
||||
// Note: cost scales with the number of open PRs in the repo.
|
||||
let reviewerLoad = {};
|
||||
try {
|
||||
const openPRs = await github.paginate(github.rest.pulls.list, {
|
||||
owner, repo, state: 'open', per_page: 100,
|
||||
});
|
||||
for (const openPR of openPRs) {
|
||||
if (openPR.number === pr_number) continue;
|
||||
for (const r of openPR.requested_reviewers) {
|
||||
if (r.login) reviewerLoad[r.login] = (reviewerLoad[r.login] || 0) + 1;
|
||||
}
|
||||
}
|
||||
} catch (e) {
|
||||
core.warning(`Could not fetch open PRs for load balancing; falling back to CODEOWNERS order: ${e.message}`);
|
||||
}
|
||||
|
||||
const isNewPR = context.payload.action === 'opened' || context.payload.action === 'reopened';
|
||||
|
||||
// On open/reopen: ignore existing assignments so GitHub's CODEOWNERS
|
||||
// auto-assignment doesn't suppress our load-balanced selection.
|
||||
// On synchronize: respect existing assignments (reviewer may have already started).
|
||||
const { selected, log } = chooseReviewers(touchedAreas, {
|
||||
prAuthor,
|
||||
existingRequested: isNewPR ? new Set() : existingRequested,
|
||||
reviewerLoad,
|
||||
LINE_THRESHOLD,
|
||||
AREA_THRESHOLDS,
|
||||
PUBLIC_HEADER,
|
||||
let reviewerLoad = {};
|
||||
try {
|
||||
const openPRs = await github.paginate(github.rest.pulls.list, {
|
||||
owner, repo, state: 'open', per_page: 100,
|
||||
});
|
||||
for (const msg of log) core.info(msg);
|
||||
|
||||
// Helper: remove CODEOWNERS auto-assigned reviewers not in our selection.
|
||||
// Only removes code owners — leaves manually-added non-owner reviewers untouched.
|
||||
async function enforceSelection(currentRequested) {
|
||||
for (const reviewer of currentRequested) {
|
||||
if (allCodeOwners.has(reviewer) && !selected.has(reviewer)) {
|
||||
try {
|
||||
await github.rest.pulls.removeRequestedReviewers({
|
||||
owner, repo, pull_number: pr_number, reviewers: [reviewer],
|
||||
});
|
||||
core.info(`Removed auto-assigned reviewer ${reviewer} (not selected by load balancer)`);
|
||||
} catch (e) {
|
||||
core.warning(`Could not remove reviewer ${reviewer}: ${e.message}`);
|
||||
}
|
||||
}
|
||||
for (const openPR of openPRs) {
|
||||
if (openPR.number === pr_number) continue;
|
||||
for (const r of openPR.requested_reviewers) {
|
||||
if (r.login) reviewerLoad[r.login] = (reviewerLoad[r.login] || 0) + 1;
|
||||
}
|
||||
}
|
||||
|
||||
if (isNewPR) {
|
||||
// First pass: clean up whatever GitHub auto-assigned before the workflow ran.
|
||||
await enforceSelection(existingRequested);
|
||||
|
||||
// Request one at a time so a single invalid login cannot block the rest.
|
||||
// Only add to confirmedRequested on success — the checklist must not show
|
||||
// an owner whose request call failed.
|
||||
for (const reviewer of selected) {
|
||||
try {
|
||||
await github.rest.pulls.requestReviewers({
|
||||
owner, repo, pull_number: pr_number,
|
||||
reviewers: [reviewer],
|
||||
});
|
||||
confirmedRequested.add(reviewer);
|
||||
} catch (e) {
|
||||
core.warning(`Could not request reviewer ${reviewer}: ${e.message}`);
|
||||
}
|
||||
}
|
||||
|
||||
// Second pass: GitHub's auto-assignment can fire after the workflow starts,
|
||||
// so wait briefly then re-check and remove any extras that appeared.
|
||||
await new Promise(resolve => setTimeout(resolve, 15000));
|
||||
let retryPR;
|
||||
try {
|
||||
({ data: retryPR } = await github.rest.pulls.get({ owner, repo, pull_number: pr_number }));
|
||||
} catch (e) {
|
||||
core.warning(`Could not re-fetch PR for reviewer cleanup retry: ${e.message}`);
|
||||
}
|
||||
if (retryPR) {
|
||||
const retryRequested = new Set(
|
||||
retryPR.requested_reviewers.map(r => r.login).filter(Boolean)
|
||||
);
|
||||
await enforceSelection(retryRequested);
|
||||
}
|
||||
} else {
|
||||
// synchronize/reopened: never re-assign reviewers — respect manual removals.
|
||||
// Just carry forward whoever is currently assigned for checklist display.
|
||||
for (const reviewer of existingRequested) confirmedRequested.add(reviewer);
|
||||
}
|
||||
} else {
|
||||
// For workflow_run (review events): show whoever is currently assigned.
|
||||
for (const reviewer of existingRequested) confirmedRequested.add(reviewer);
|
||||
} catch (e) {
|
||||
core.warning(`Could not fetch open PRs for load balancing; falling back to CODEOWNERS order: ${e.message}`);
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 7. Build the checklist body.
|
||||
// 7. Coordinate reviewer assignment → confirmed set for display.
|
||||
// ----------------------------------------------------------------
|
||||
const confirmedRequested = await coordinateReviewers(github, context, core, {
|
||||
owner, repo, pr_number, prData, allCodeOwners, touchedAreas, reviewerLoad,
|
||||
LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER,
|
||||
});
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 8. Build and post (or update) the checklist comment.
|
||||
// ----------------------------------------------------------------
|
||||
const body = buildBody(touchedAreas, approvedUsers, confirmedRequested);
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// 8. Create or update the checklist comment (idempotent via marker).
|
||||
// ----------------------------------------------------------------
|
||||
try {
|
||||
const comments = await github.paginate(github.rest.issues.listComments, {
|
||||
owner, repo, issue_number: pr_number, per_page: 100,
|
||||
@@ -503,14 +536,10 @@ module.exports = async function run({ github, context, core }) {
|
||||
const existing = comments.find(c => c.body.includes(MARKER));
|
||||
|
||||
if (existing) {
|
||||
await github.rest.issues.updateComment({
|
||||
owner, repo, comment_id: existing.id, body,
|
||||
});
|
||||
await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body });
|
||||
core.info(`Updated checklist comment #${existing.id}`);
|
||||
} else {
|
||||
await github.rest.issues.createComment({
|
||||
owner, repo, issue_number: pr_number, body,
|
||||
});
|
||||
await github.rest.issues.createComment({ owner, repo, issue_number: pr_number, body });
|
||||
core.info('Created checklist comment');
|
||||
}
|
||||
} catch (error) {
|
||||
|
||||
@@ -0,0 +1,104 @@
|
||||
name: Remove Reviewer
|
||||
|
||||
# Responds to /remove-reviewer @username comments on PRs.
|
||||
# Only collaborators with write access or above can use the command.
|
||||
#
|
||||
# Usage (in any PR comment):
|
||||
# /remove-reviewer @jhendersonHDF
|
||||
# /remove-reviewer @jhendersonHDF @mattjala ← multiple at once
|
||||
|
||||
on:
|
||||
issue_comment:
|
||||
types: [created]
|
||||
|
||||
permissions:
|
||||
pull-requests: write
|
||||
issues: write
|
||||
|
||||
jobs:
|
||||
remove:
|
||||
runs-on: ubuntu-latest
|
||||
# Only fire on PR comments in the main repo that contain the slash command
|
||||
if: |
|
||||
github.repository == 'HDFGroup/hdf5' &&
|
||||
github.event.issue.pull_request != null &&
|
||||
contains(github.event.comment.body, '/remove-reviewer')
|
||||
|
||||
steps:
|
||||
- uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0
|
||||
with:
|
||||
script: |
|
||||
const body = context.payload.comment.body;
|
||||
const commenter = context.payload.comment.user.login;
|
||||
const { owner, repo } = context.repo;
|
||||
const pr_number = context.payload.issue.number;
|
||||
|
||||
// Parse all @usernames after /remove-reviewer
|
||||
const match = body.match(/\/remove-reviewer((?:\s+@[\w-]+)+)/i);
|
||||
if (!match) return;
|
||||
|
||||
const toRemove = [...match[1].matchAll(/@([\w-]+)/g)].map(m => m[1]);
|
||||
if (toRemove.length === 0) return;
|
||||
|
||||
// Acknowledge the command with a reaction so the team knows it ran
|
||||
await github.rest.reactions.createForIssueComment({
|
||||
owner, repo,
|
||||
comment_id: context.payload.comment.id,
|
||||
content: 'eyes',
|
||||
}).catch(() => {});
|
||||
|
||||
// Security: commenter must have write or admin access
|
||||
let permission;
|
||||
try {
|
||||
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
|
||||
owner, repo, username: commenter,
|
||||
});
|
||||
permission = data.permission;
|
||||
} catch {
|
||||
permission = 'none';
|
||||
}
|
||||
|
||||
if (!['write', 'admin'].includes(permission)) {
|
||||
await github.rest.issues.createComment({
|
||||
owner, repo, issue_number: pr_number,
|
||||
body: `@${commenter} — write access is required to remove reviewers.`,
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
// Fetch all reviews to detect who has already submitted one
|
||||
const reviews = await github.paginate(github.rest.pulls.listReviews, {
|
||||
owner, repo, pull_number: pr_number, per_page: 100,
|
||||
});
|
||||
const hasReviewed = new Set(reviews.map(r => r.user.login));
|
||||
|
||||
// Remove each requested reviewer
|
||||
const removed = [], alreadyReviewed = [], failed = [];
|
||||
for (const username of toRemove) {
|
||||
if (hasReviewed.has(username)) {
|
||||
alreadyReviewed.push(username);
|
||||
continue;
|
||||
}
|
||||
try {
|
||||
await github.rest.pulls.removeRequestedReviewers({
|
||||
owner, repo, pull_number: pr_number,
|
||||
reviewers: [username],
|
||||
});
|
||||
removed.push(username);
|
||||
} catch (e) {
|
||||
failed.push(`${username} (${e.message})`);
|
||||
}
|
||||
}
|
||||
|
||||
// Post a single summary reply
|
||||
const lines = [];
|
||||
if (removed.length)
|
||||
lines.push(`Removed: ${removed.map(u => `@${u}`).join(', ')}`);
|
||||
if (alreadyReviewed.length)
|
||||
lines.push(`Cannot remove (already reviewed): ${alreadyReviewed.map(u => `@${u}`).join(', ')}`);
|
||||
if (failed.length)
|
||||
lines.push(`Could not remove: ${failed.join(', ')}`);
|
||||
await github.rest.issues.createComment({
|
||||
owner, repo, issue_number: pr_number,
|
||||
body: lines.join('\n'),
|
||||
});
|
||||
@@ -23,7 +23,7 @@ on:
|
||||
# Safe: checkout has no ref: override so the base branch (develop) is always
|
||||
# used — the fork's code is never checked out or executed.
|
||||
pull_request_target:
|
||||
types: [opened, synchronize, reopened, review_requested, review_request_removed]
|
||||
types: [opened, synchronize, reopened, ready_for_review]
|
||||
branches: [develop]
|
||||
workflow_run:
|
||||
workflows: ["Review Checklist (gather)"]
|
||||
|
||||
Reference in New Issue
Block a user