diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index aedd979961..13f4c6bfb6 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -1,10 +1,14 @@ const path = require("path"); +const { execSync } = require("child_process"); const { parseOwnersFile, + findOwners, getMaintainers, getOwnershipGroups, } = require("../scripts/owners"); +// --- Approval helpers --- + /** * Check if an approver is a member of a GitHub team. * Requires org read access on the token; falls back to false if unavailable. @@ -19,10 +23,8 @@ async function isTeamMember(github, org, teamSlug, login, core) { return data.state === "active"; } catch (err) { if (err.status === 404) { - // User is genuinely not a member of this team. return false; } - // Permission denied or other error. Log it so it's visible. if (core) { core.warning( `Could not verify team membership for ${login} in ${org}/${teamSlug} ` + @@ -35,56 +37,380 @@ async function isTeamMember(github, org, teamSlug, login, core) { } /** - * Check if any approver matches an owner entry. - * Owner can be a plain login or an org/team ref (containing "/"). + * Find which approver (if any) satisfies a group's ownership requirement. + * Returns the login of the first matching approver, or null. */ -async function ownerHasApproval(owner, approverSet, github, org, core) { - if (owner.includes("/")) { - const teamSlug = owner.split("/")[1]; - for (const approver of approverSet) { - if (await isTeamMember(github, org, teamSlug, approver, core)) { - return true; +async function findGroupApprover(owners, approverLogins, github, org, core) { + const approverSet = new Set(approverLogins.map(l => l.toLowerCase())); + for (const owner of owners) { + if (owner.includes("/")) { + const teamSlug = owner.split("/")[1]; + for (const approver of approverLogins) { + if (await isTeamMember(github, org, teamSlug, approver, core)) { + return approver; + } } + } else if (approverSet.has(owner.toLowerCase())) { + return owner; } - return false; } - return approverSet.has(owner.toLowerCase()); + return null; } /** * Per-path approval check. Each ownership group needs at least one * approval from its owners. Files matching only "*" require a maintainer. + * Returns groups, approvedBy map, and coverage info. */ async function checkPerPathApproval(files, rulesWithTeams, approverLogins, github, org, core) { const groups = getOwnershipGroups(files.map(f => f.filename), rulesWithTeams); - const approverSet = new Set(approverLogins.map(l => l.toLowerCase())); + const approvedBy = new Map(); if (groups.has("*")) { + // Still check non-wildcard groups for comment building + for (const [pattern, { owners }] of groups) { + if (pattern === "*") continue; + const approver = await findGroupApprover(owners, approverLogins, github, org, core); + if (approver) approvedBy.set(pattern, approver); + } return { allCovered: false, hasWildcardFiles: true, wildcardFiles: groups.get("*").files, + groups, + approvedBy, }; } const uncovered = []; for (const [pattern, { owners }] of groups) { - let hasApproval = false; - for (const owner of owners) { - if (await ownerHasApproval(owner, approverSet, github, org, core)) { - hasApproval = true; - break; - } - } - if (!hasApproval) { + const approver = await findGroupApprover(owners, approverLogins, github, org, core); + if (approver) { + approvedBy.set(pattern, approver); + } else { uncovered.push({ pattern, owners }); } } - return { allCovered: uncovered.length === 0, uncovered }; + return { allCovered: uncovered.length === 0, uncovered, groups, approvedBy }; } +// --- Git history & scoring helpers --- + +const MENTION_REVIEWERS = true; +const OWNERS_LINK = "[OWNERS](.github/OWNERS)"; +const MARKER = ""; const STATUS_CONTEXT = "maintainer-approval"; +const loginCache = {}; + +function classifyFile(filepath, totalFiles) { + const base = path.basename(filepath); + if (base.startsWith("out.") || base === "output.txt") { + return 0.01 / Math.max(totalFiles, 1); + } + if (filepath.startsWith("acceptance/") || filepath.startsWith("integration/")) { + return 0.2; + } + if (filepath.endsWith("_test.go")) return 0.3; + return filepath.endsWith(".go") ? 1.0 : 0.5; +} + +function gitLog(filepath) { + try { + const out = execSync( + `git log -50 --no-merges --since="12 months ago" --format="%H|%an|%aI" -- "${filepath}"`, + { encoding: "utf-8" } + ); + const entries = []; + for (const line of out.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) continue; + const parts = trimmed.split("|", 3); + if (parts.length !== 3) continue; + const date = new Date(parts[2]); + if (isNaN(date.getTime())) continue; + entries.push({ sha: parts[0], name: parts[1], date }); + } + return entries; + } catch { + return []; + } +} + +async function resolveLogin(github, owner, repo, sha, authorName) { + if (authorName in loginCache) return loginCache[authorName]; + try { + const { data } = await github.rest.repos.getCommit({ owner, repo, ref: sha }); + const login = data.author?.login || null; + loginCache[authorName] = login; + return login; + } catch { + loginCache[authorName] = null; + return null; + } +} + +function parseOwnersForFiles(changedFiles, ownersPath) { + const rules = parseOwnersFile(ownersPath, { includeTeams: true }); + const allOwners = new Set(); + for (const filepath of changedFiles) { + for (const o of findOwners(filepath, rules)) allOwners.add(o); + } + return Array.from(allOwners).sort(); +} + +async function scoreContributors(files, prAuthor, now, github, owner, repo) { + const scores = {}; + const dirScores = {}; + let scoredCount = 0; + const authorLogin = (prAuthor || "").toLowerCase(); + const totalFiles = files.length; + + for (const filepath of files) { + const weight = classifyFile(filepath, totalFiles); + let history = gitLog(filepath); + if (history.length === 0) { + const parent = path.dirname(filepath); + if (parent && parent !== ".") { + history = gitLog(parent); + } + } + if (history.length === 0) continue; + + const topDir = path.dirname(filepath) || "."; + let fileContributed = false; + for (const { sha, name, date } of history) { + if (name.endsWith("[bot]")) continue; + const login = await resolveLogin(github, owner, repo, sha, name); + if (!login || login.toLowerCase() === authorLogin) continue; + const daysAgo = Math.max(0, (now - date) / 86400000); + const s = weight * Math.pow(0.5, daysAgo / 150); + scores[login] = (scores[login] || 0) + s; + if (!dirScores[login]) dirScores[login] = {}; + dirScores[login][topDir] = (dirScores[login][topDir] || 0) + s; + fileContributed = true; + } + if (fileContributed) scoredCount++; + } + return { scores, dirScores, scoredCount }; +} + +function topDirs(ds, n = 3) { + return Object.entries(ds || {}) + .sort((a, b) => b[1] - a[1]) + .slice(0, n) + .map(([d]) => d); +} + +function fmtReviewer(login, dirs) { + const mention = MENTION_REVIEWERS ? `@${login}` : login; + const dirList = dirs.map((d) => `\`${d}/\``).join(", "); + return `- ${mention} -- recent work in ${dirList}`; +} + +function selectReviewers(ss) { + if (ss.length === 0) return []; + const out = [ss[0]]; + if (ss.length >= 2 && ss[0][1] < 1.5 * ss[1][1]) { + out.push(ss[1]); + if (ss.length >= 3 && ss[1][1] < 1.5 * ss[2][1]) { + out.push(ss[2]); + } + } + return out; +} + +function fmtEligible(owners) { + if (MENTION_REVIEWERS) return owners.map((o) => `@${o}`).join(", "); + return owners.join(", "); +} + +async function countRecentReviews(github, owner, repo, logins, days = 30) { + const since = new Date(Date.now() - days * 86400000) + .toISOString() + .slice(0, 10); + const counts = {}; + for (const login of logins) { + try { + const { data } = await github.rest.search.issuesAndPullRequests({ + q: `repo:${owner}/${repo} reviewed-by:${login} is:pr created:>${since}`, + }); + counts[login] = data.total_count; + } catch { + // skip on error + } + } + return counts; +} + +async function selectRoundRobin(github, owner, repo, eligibleOwners, prAuthor) { + const candidates = eligibleOwners + .filter((o) => !o.includes("/") && o.toLowerCase() !== (prAuthor || "").toLowerCase()); + if (candidates.length === 0) return null; + const counts = await countRecentReviews(github, owner, repo, candidates); + if (Object.keys(counts).length === 0) { + return candidates[Math.floor(Math.random() * candidates.length)]; + } + return candidates.reduce((best, c) => + (counts[c] || 0) < (counts[best] || 0) ? c : best + ); +} + +// --- Comment builders --- + +function buildApprovedComment(description) { + const lines = [ + MARKER, + `## ${description}`, + "", + `See ${OWNERS_LINK} for ownership rules.`, + ]; + return lines.join("\n") + "\n"; +} + +function buildAllGroupsApprovedComment(groups, approvedBy) { + const lines = [MARKER, "## All ownership groups approved", ""]; + for (const [pattern, { files }] of groups) { + if (pattern === "*") continue; + lines.push(`### \`${pattern}\` - approved`); + lines.push(`Files: ${files.map(f => `\`${f}\``).join(", ")}`); + const approver = approvedBy.get(pattern); + if (approver) { + lines.push(`Approved by: @${approver}`); + } + lines.push(""); + } + lines.push(`See ${OWNERS_LINK} for ownership rules.`); + return lines.join("\n") + "\n"; +} + +function buildPendingPerGroupComment(groups, scores, dirScores, approvedBy, maintainers, prAuthor) { + const authorLower = (prAuthor || "").toLowerCase(); + const lines = [MARKER, "## Approval status: pending", ""]; + + for (const [pattern, { owners, files }] of groups) { + if (pattern === "*") continue; + + const approver = approvedBy.get(pattern); + if (approver) { + lines.push(`### \`${pattern}\` - approved by @${approver}`); + } else { + lines.push(`### \`${pattern}\` - needs approval`); + } + lines.push(`Files: ${files.map(f => `\`${f}\``).join(", ")}`); + + const teams = owners.filter(o => o.includes("/")); + const individuals = owners.filter(o => !o.includes("/") && o.toLowerCase() !== authorLower); + + if (teams.length > 0) { + lines.push(`Teams: ${teams.map(t => `@${t}`).join(", ")}`); + } + + if (!approver && individuals.length > 0) { + const scored = individuals.map(o => [o, scores[o] || 0]).sort((a, b) => b[1] - a[1]); + if (scored[0][1] > 0) { + lines.push(`Suggested: @${scored[0][0]}`); + const rest = scored.slice(1).map(([o]) => o); + if (rest.length > 0) { + lines.push(`Also eligible: ${fmtEligible(rest)}`); + } + } else { + lines.push(`Eligible: ${fmtEligible(individuals)}`); + } + } + lines.push(""); + } + + const starGroup = groups.get("*"); + if (starGroup) { + lines.push("### General files (require maintainer)"); + lines.push(`Files: ${starGroup.files.map(f => `\`${f}\``).join(", ")}`); + + const maintainerSet = new Set(maintainers.map(m => m.toLowerCase())); + const maintainerScores = Object.entries(scores) + .filter(([login]) => + login.toLowerCase() !== authorLower && maintainerSet.has(login.toLowerCase()) + ) + .sort((a, b) => b[1] - a[1]); + + if (maintainerScores.length > 0 && maintainerScores[0][1] > 0) { + const [login] = maintainerScores[0]; + const dirs = topDirs(dirScores[login]); + lines.push("Based on git history:"); + lines.push(fmtReviewer(login, dirs)); + } else { + lines.push(`Pick a maintainer from ${OWNERS_LINK}.`); + } + lines.push(""); + } + + const maintainerList = maintainers + .filter(m => m.toLowerCase() !== authorLower) + .map(m => `@${m}`) + .join(", "); + + lines.push( + `Any maintainer (${maintainerList}) can approve all areas.`, + `See ${OWNERS_LINK} for ownership rules.` + ); + + return lines.join("\n") + "\n"; +} + +function buildSingleDomainPendingComment(sortedScores, dirScores, scoredCount, eligibleOwners, prAuthor, roundRobinReviewer) { + const reviewers = selectReviewers(sortedScores); + const suggestedLogins = new Set(reviewers.map(([login]) => login.toLowerCase())); + const eligible = eligibleOwners.filter( + o => o.toLowerCase() !== (prAuthor || "").toLowerCase() && !suggestedLogins.has(o.toLowerCase()) + ); + + const lines = [MARKER, "## Waiting for approval", ""]; + + if (reviewers.length > 0) { + lines.push("Based on git history, these people are best suited to review:", ""); + for (const [login] of reviewers) { + lines.push(fmtReviewer(login, topDirs(dirScores[login]))); + } + lines.push(""); + } else if (roundRobinReviewer) { + lines.push( + "Could not determine reviewers from git history.", + `Round-robin suggestion: @${roundRobinReviewer}`, + "" + ); + } + + if (eligible.length > 0) { + lines.push(`Eligible reviewers: ${fmtEligible(eligible)}`, ""); + } + + lines.push(`Suggestions based on git history. See ${OWNERS_LINK} for ownership rules.`); + return lines.join("\n") + "\n"; +} + +// --- Comment management --- + +const LEGACY_MARKER = ""; + +async function postComment(github, owner, repo, prNumber, comment) { + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: prNumber, + }); + const toDelete = comments.filter(c => + c.body && (c.body.includes(MARKER) || c.body.includes(LEGACY_MARKER)) + ); + for (const c of toDelete) { + await github.rest.issues.deleteComment({ + owner, repo, comment_id: c.id, + }); + } + await github.rest.issues.createComment({ + owner, repo, issue_number: prNumber, body: comment, + }); +} + +// --- Main --- + module.exports = async ({ github, context, core }) => { const ownersPath = path.join( process.env.GITHUB_WORKSPACE, @@ -102,6 +428,9 @@ module.exports = async ({ github, context, core }) => { } const { pull_request: pr } = context.payload; + const owner = context.repo.owner; + const repo = context.repo.repo; + const prNumber = context.issue.number; const authorLogin = pr?.user?.login; const sha = pr.head.sha; const statusParams = { @@ -117,26 +446,25 @@ module.exports = async ({ github, context, core }) => { pull_number: context.issue.number, }); - const maintainerApproved = reviews.some( + // Maintainer approval -> success with simple comment + const maintainerApproval = reviews.find( ({ state, user }) => state === "APPROVED" && user && maintainers.includes(user.login) ); - - if (maintainerApproved) { - const approver = reviews.find( - ({ state, user }) => - state === "APPROVED" && user && maintainers.includes(user.login) - ); - core.info(`Maintainer approval from @${approver.user.login}`); + if (maintainerApproval) { + const approver = maintainerApproval.user.login; + core.info(`Maintainer approval from @${approver}`); await github.rest.repos.createCommitStatus({ ...statusParams, state: "success", - description: `Approved by @${approver.user.login}`, + description: `Approved by @${approver}`, }); + await postComment(github, owner, repo, prNumber, + buildApprovedComment(`Approved by @${approver}`)); return; } - // If the PR author is a maintainer, any approval suffices (second pair of eyes). + // Maintainer-authored PR with any approval -> success if (authorLogin && maintainers.includes(authorLogin)) { const hasAnyApproval = reviews.some( ({ state, user }) => @@ -149,12 +477,13 @@ module.exports = async ({ github, context, core }) => { state: "success", description: "Approved (maintainer-authored PR)", }); + await postComment(github, owner, repo, prNumber, + buildApprovedComment("Approved (maintainer-authored PR)")); return; } } - // Gather approved logins (excluding the PR author, since GitHub prevents - // self-approval, but we filter defensively in case of API edge cases). + // Gather approved logins (excluding the PR author). const approverLogins = reviews .filter( ({ state, user }) => @@ -177,6 +506,7 @@ module.exports = async ({ github, context, core }) => { core ); + // Set commit status if (result.allCovered && approverLogins.length > 0) { core.info("All ownership groups have per-path approval."); await github.rest.repos.createCommitStatus({ @@ -184,10 +514,7 @@ module.exports = async ({ github, context, core }) => { state: "success", description: "All ownership groups approved", }); - return; - } - - if (result.hasWildcardFiles) { + } else if (result.hasWildcardFiles) { const fileList = result.wildcardFiles.join(", "); const msg = `Files need maintainer review: ${fileList}. ` + @@ -198,10 +525,7 @@ module.exports = async ({ github, context, core }) => { state: "pending", description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg, }); - return; - } - - if (result.uncovered && result.uncovered.length > 0) { + } else if (result.uncovered && result.uncovered.length > 0) { const groupList = result.uncovered .map(({ pattern, owners }) => `${pattern} (needs: ${owners.join(", ")})`) .join("; "); @@ -214,14 +538,50 @@ module.exports = async ({ github, context, core }) => { state: "pending", description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg, }); - return; + } else { + const msg = `Waiting for maintainer approval: ${maintainers.join(", ")}`; + core.info(msg); + await github.rest.repos.createCommitStatus({ + ...statusParams, + state: "pending", + description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg, + }); } - const msg = `Waiting for maintainer approval: ${maintainers.join(", ")}`; - core.info(msg); - await github.rest.repos.createCommitStatus({ - ...statusParams, - state: "pending", - description: msg.length > 140 ? msg.slice(0, 137) + "..." : msg, - }); + // Score contributors via git history + const fileNames = files.map(f => f.filename); + const now = new Date(); + const { scores, dirScores, scoredCount } = await scoreContributors( + fileNames, + authorLogin, + now, + github, + owner, + repo + ); + const sortedScores = Object.entries(scores).sort((a, b) => b[1] - a[1]); + + // Build comment based on approval state and ownership groups + let comment; + const groups = result.groups; + + if (result.allCovered && approverLogins.length > 0) { + comment = buildAllGroupsApprovedComment(groups, result.approvedBy); + } else if (groups.size >= 2) { + comment = buildPendingPerGroupComment( + groups, scores, dirScores, result.approvedBy, maintainers, authorLogin + ); + } else { + const eligible = parseOwnersForFiles(fileNames, ownersPath); + let roundRobin = null; + if (selectReviewers(sortedScores).length === 0 && eligible.length > 0) { + roundRobin = await selectRoundRobin(github, owner, repo, eligible, authorLogin); + } + comment = buildSingleDomainPendingComment( + sortedScores, dirScores, scoredCount, eligible, authorLogin, roundRobin + ); + } + + core.info(comment); + await postComment(github, owner, repo, prNumber, comment); }; diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js index da3f0967ea..3c38ddc10d 100644 --- a/.github/workflows/maintainer-approval.test.js +++ b/.github/workflows/maintainer-approval.test.js @@ -1,4 +1,4 @@ -const { describe, it, before, after, beforeEach } = require("node:test"); +const { describe, it, before, after } = require("node:test"); const assert = require("node:assert/strict"); const fs = require("fs"); const os = require("os"); @@ -29,6 +29,7 @@ function makeContext({ author = "someuser", sha = "abc123", prNumber = 42 } = {} issue: { number: prNumber }, payload: { pull_request: { + number: prNumber, user: { login: author }, head: { sha }, }, @@ -53,16 +54,21 @@ function makeCore() { * @param {Array} opts.reviews - PR reviews to return * @param {Array} opts.files - PR files to return (objects with .filename) * @param {Object} opts.teamMembers - { teamSlug: [logins] } + * @param {Array} opts.existingComments - Existing PR comments to return */ -function makeGithub({ reviews = [], files = [], teamMembers = {} } = {}) { +function makeGithub({ reviews = [], files = [], teamMembers = {}, existingComments = [] } = {}) { const listReviews = Symbol("listReviews"); const listFiles = Symbol("listFiles"); + const listComments = Symbol("listComments"); const statuses = []; + const createdComments = []; + const deletedCommentIds = []; const github = { paginate: async (endpoint, _opts) => { if (endpoint === listReviews) return reviews; if (endpoint === listFiles) return files; + if (endpoint === listComments) return existingComments; return []; }, rest: { @@ -75,6 +81,15 @@ function makeGithub({ reviews = [], files = [], teamMembers = {} } = {}) { statuses.push(params); }, }, + issues: { + listComments, + deleteComment: async (params) => { + deletedCommentIds.push(params.comment_id); + }, + createComment: async (params) => { + createdComments.push(params); + }, + }, teams: { getMembershipForUserInOrg: async ({ team_slug, username }) => { if (teamMembers[team_slug]?.includes(username)) { @@ -87,6 +102,8 @@ function makeGithub({ reviews = [], files = [], teamMembers = {} } = {}) { }, }, _statuses: statuses, + _comments: createdComments, + _deletedCommentIds: deletedCommentIds, }; return github; } @@ -319,4 +336,134 @@ describe("maintainer-approval", () => { process.env.GITHUB_WORKSPACE = oldWorkspace; fs.rmSync(noWildcardDir, { recursive: true }); }); + + // --- Comment posting tests --- + + it("posts a comment with MARKER on every run", async () => { + const github = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._comments.length, 1); + assert.ok(github._comments[0].body.includes("")); + }); + + it("maintainer approval posts simple approved comment", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "maintainer1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._comments.length, 1); + assert.ok(github._comments[0].body.includes("Approved by @maintainer1")); + assert.ok(github._comments[0].body.includes("")); + }); + + it("deletes existing comment before posting new one", async () => { + const github = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + existingComments: [ + { id: 999, body: "\nOld comment" }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._deletedCommentIds.length, 1); + assert.equal(github._deletedCommentIds[0], 999); + assert.equal(github._comments.length, 1); + assert.ok(github._comments[0].body.includes("")); + }); + + it("does not delete comments without the marker", async () => { + const github = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + existingComments: [ + { id: 888, body: "Some unrelated comment" }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._deletedCommentIds.length, 0); + assert.equal(github._comments.length, 1); + }); + + it("pending single-domain comment includes waiting header", async () => { + const github = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._comments.length, 1); + assert.ok(github._comments[0].body.includes("## Waiting for approval")); + }); + + it("pending cross-domain comment includes per-group sections", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + ], + files: [ + { filename: "cmd/pipelines/foo.go" }, + { filename: "bundle/config.go" }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._comments.length, 1); + const body = github._comments[0].body; + assert.ok(body.includes("## Approval status: pending")); + assert.ok(body.includes("`/cmd/pipelines/`")); + assert.ok(body.includes("`/bundle/`")); + assert.ok(body.includes("approved by @jefferycheng1")); + assert.ok(body.includes("needs approval")); + }); + + it("all groups approved comment shows per-group detail", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + { state: "APPROVED", user: { login: "bundleowner" } }, + ], + files: [ + { filename: "cmd/pipelines/foo.go" }, + { filename: "bundle/config.go" }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._comments.length, 1); + const body = github._comments[0].body; + assert.ok(body.includes("## All ownership groups approved")); + assert.ok(body.includes("Approved by: @jefferycheng1")); + assert.ok(body.includes("Approved by: @bundleowner")); + }); }); diff --git a/.github/workflows/maintainer-approval.yml b/.github/workflows/maintainer-approval.yml index d48086f0dc..b33fad48c5 100644 --- a/.github/workflows/maintainer-approval.yml +++ b/.github/workflows/maintainer-approval.yml @@ -1,10 +1,15 @@ -name: Maintainer approval +name: PR approval on: pull_request_target: + types: [opened, synchronize, reopened, ready_for_review] pull_request_review: types: [submitted, dismissed] +concurrency: + group: pr-approval-${{ github.event.pull_request.number }} + cancel-in-progress: true + defaults: run: shell: bash @@ -14,22 +19,19 @@ jobs: runs-on: group: databricks-deco-testing-runner-group labels: ubuntu-latest-deco + if: ${{ !github.event.pull_request.draft }} timeout-minutes: 5 - # Note: team membership resolution in per-path approval requires a token - # with org:read scope. The default GITHUB_TOKEN may lack this, causing - # team-based ownership checks to fall back to maintainer approval. permissions: - pull-requests: read + pull-requests: write statuses: write + contents: read steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: + ref: ${{ github.event.pull_request.base.sha }} persist-credentials: false - sparse-checkout: | - .github/workflows - .github/scripts - .github/OWNERS - - name: Require core team approval + fetch-depth: 0 + - name: Check approval and suggest reviewers uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 with: retries: 3 diff --git a/.github/workflows/suggest-reviewers.js b/.github/workflows/suggest-reviewers.js deleted file mode 100644 index efac10a335..0000000000 --- a/.github/workflows/suggest-reviewers.js +++ /dev/null @@ -1,461 +0,0 @@ -const path = require("path"); -const { execSync } = require("child_process"); -const { - parseOwnersFile, - findOwners, - getMaintainers, - getOwnershipGroups, -} = require("../scripts/owners"); - -const MENTION_REVIEWERS = true; -const OWNERS_LINK = "[OWNERS](.github/OWNERS)"; -const MARKER = ""; - -const loginCache = {}; - -function classifyFile(filepath, totalFiles) { - const base = path.basename(filepath); - if (base.startsWith("out.") || base === "output.txt") { - return 0.01 / Math.max(totalFiles, 1); - } - if (filepath.startsWith("acceptance/") || filepath.startsWith("integration/")) { - return 0.2; - } - if (filepath.endsWith("_test.go")) return 0.3; - return filepath.endsWith(".go") ? 1.0 : 0.5; -} - -async function getChangedFiles(github, owner, repo, prNumber) { - const files = await github.paginate(github.rest.pulls.listFiles, { - owner, - repo, - pull_number: prNumber, - }); - return files.map((f) => f.filename); -} - -function gitLog(filepath) { - try { - const out = execSync( - `git log -50 --no-merges --since="12 months ago" --format="%H|%an|%aI" -- "${filepath}"`, - { encoding: "utf-8" } - ); - const entries = []; - for (const line of out.split("\n")) { - const trimmed = line.trim(); - if (!trimmed) continue; - const parts = trimmed.split("|", 3); - if (parts.length !== 3) continue; - const date = new Date(parts[2]); - if (isNaN(date.getTime())) continue; - entries.push({ sha: parts[0], name: parts[1], date }); - } - return entries; - } catch { - return []; - } -} - -async function resolveLogin(github, owner, repo, sha, authorName) { - if (authorName in loginCache) return loginCache[authorName]; - try { - const { data } = await github.rest.repos.getCommit({ owner, repo, ref: sha }); - const login = data.author?.login || null; - loginCache[authorName] = login; - return login; - } catch { - loginCache[authorName] = null; - return null; - } -} - -function parseOwnersForFiles(changedFiles, ownersPath) { - const rules = parseOwnersFile(ownersPath, { includeTeams: true }); - const allOwners = new Set(); - for (const filepath of changedFiles) { - for (const o of findOwners(filepath, rules)) allOwners.add(o); - } - return Array.from(allOwners).sort(); -} - -async function scoreContributors(files, prAuthor, now, github, owner, repo) { - const scores = {}; - const dirScores = {}; - let scoredCount = 0; - const authorLogin = prAuthor.toLowerCase(); - const totalFiles = files.length; - - for (const filepath of files) { - const weight = classifyFile(filepath, totalFiles); - let history = gitLog(filepath); - if (history.length === 0) { - const parent = path.dirname(filepath); - if (parent && parent !== ".") { - history = gitLog(parent); - } - } - if (history.length === 0) continue; - - const topDir = path.dirname(filepath) || "."; - let fileContributed = false; - for (const { sha, name, date } of history) { - if (name.endsWith("[bot]")) continue; - const login = await resolveLogin(github, owner, repo, sha, name); - if (!login || login.toLowerCase() === authorLogin) continue; - const daysAgo = Math.max(0, (now - date) / 86400000); - const s = weight * Math.pow(0.5, daysAgo / 150); - scores[login] = (scores[login] || 0) + s; - if (!dirScores[login]) dirScores[login] = {}; - dirScores[login][topDir] = (dirScores[login][topDir] || 0) + s; - fileContributed = true; - } - if (fileContributed) scoredCount++; - } - return { scores, dirScores, scoredCount }; -} - -function topDirs(ds, n = 3) { - return Object.entries(ds || {}) - .sort((a, b) => b[1] - a[1]) - .slice(0, n) - .map(([d]) => d); -} - -function fmtReviewer(login, dirs) { - const mention = MENTION_REVIEWERS ? `@${login}` : login; - const dirList = dirs.map((d) => `\`${d}/\``).join(", "); - return `- ${mention} -- recent work in ${dirList}`; -} - -function selectReviewers(ss) { - if (ss.length === 0) return []; - const out = [ss[0]]; - if (ss.length >= 2 && ss[0][1] < 1.5 * ss[1][1]) { - out.push(ss[1]); - if (ss.length >= 3 && ss[1][1] < 1.5 * ss[2][1]) { - out.push(ss[2]); - } - } - return out; -} - -function computeConfidence(ss, scoredCount) { - if (ss.length === 0) return "low"; - if (ss.length === 1) return "high"; - if (ss.length === 2) { - if (ss[0][1] > 2 * ss[1][1]) return "high"; - if (ss[0][1] > 1.5 * ss[1][1]) return "medium"; - return "low"; - } - if (scoredCount < 3) return "low"; - if (ss[0][1] > 2 * ss[2][1]) return "high"; - if (ss[0][1] > 1.5 * ss[2][1]) return "medium"; - return "low"; -} - -function fmtEligible(owners) { - if (MENTION_REVIEWERS) return owners.map((o) => `@${o}`).join(", "); - return owners.join(", "); -} - -async function countRecentReviews(github, owner, repo, logins, days = 30) { - const since = new Date(Date.now() - days * 86400000) - .toISOString() - .slice(0, 10); - const counts = {}; - for (const login of logins) { - try { - const { data } = await github.rest.search.issuesAndPullRequests({ - q: `repo:${owner}/${repo} reviewed-by:${login} is:pr created:>${since}`, - }); - counts[login] = data.total_count; - } catch { - // skip on error - } - } - return counts; -} - -async function selectRoundRobin(github, owner, repo, eligibleOwners, prAuthor) { - const candidates = eligibleOwners - .filter((o) => !o.includes("/") && o.toLowerCase() !== prAuthor.toLowerCase()); - if (candidates.length === 0) return null; - const counts = await countRecentReviews(github, owner, repo, candidates); - if (Object.keys(counts).length === 0) { - return candidates[Math.floor(Math.random() * candidates.length)]; - } - return candidates.reduce((best, c) => - (counts[c] || 0) < (counts[best] || 0) ? c : best - ); -} - -function buildComment( - sortedScores, - dirScores, - totalFiles, - scoredCount, - eligibleOwners, - prAuthor, - roundRobinReviewer -) { - const reviewers = selectReviewers(sortedScores); - const suggestedLogins = new Set(reviewers.map(([login]) => login.toLowerCase())); - const eligible = eligibleOwners.filter( - (o) => - o.toLowerCase() !== prAuthor.toLowerCase() && - !suggestedLogins.has(o.toLowerCase()) - ); - - const lines = [MARKER]; - if (reviewers.length > 0) { - lines.push( - "## Suggested reviewers", - "", - "Based on git history of the changed files, these people are best suited to review:", - "" - ); - for (const [login] of reviewers) { - lines.push(fmtReviewer(login, topDirs(dirScores[login]))); - } - lines.push("", `Confidence: ${computeConfidence(sortedScores, scoredCount)}`); - if (eligible.length > 0) { - lines.push( - "", - "## Eligible reviewers", - "", - "Based on OWNERS, these people or teams could also review:", - "", - fmtEligible(eligible) - ); - } - } else if (eligible.length > 0) { - if (roundRobinReviewer) { - const mention = MENTION_REVIEWERS - ? `@${roundRobinReviewer}` - : roundRobinReviewer; - const remaining = eligible.filter( - (o) => o.toLowerCase() !== roundRobinReviewer.toLowerCase() - ); - lines.push( - "## Suggested reviewer", - "", - "Could not determine reviewers from git history.", - "Round-robin suggestion (based on recent review load):", - "", - `- ${mention}` - ); - if (remaining.length > 0) { - lines.push( - "", - "## Eligible reviewers", - "", - "Based on OWNERS, these people or teams could also review:", - "", - fmtEligible(remaining) - ); - } - } else { - lines.push( - "## Eligible reviewers", - "", - "Could not determine reviewers from git history. Based on OWNERS, these people or teams could review:", - "", - fmtEligible(eligible) - ); - } - } else { - lines.push( - "## Suggested reviewers", - "", - `Could not determine reviewers from git history. Please pick from ${OWNERS_LINK}.` - ); - } - - lines.push( - "", - `Suggestions based on git history of ${totalFiles} changed files ` + - `(${scoredCount} scored). ` + - `See ${OWNERS_LINK} for path-specific ownership rules.` - ); - return lines.join("\n") + "\n"; -} - -function buildPerGroupComment( - groups, - scores, - dirScores, - maintainers, - prAuthor, - totalFiles -) { - const authorLower = prAuthor.toLowerCase(); - const lines = [MARKER, "## Reviewers by ownership area", ""]; - lines.push( - "This PR crosses multiple ownership areas. Each area needs at least one", - "approval from its owners (or any maintainer can approve everything).", - "" - ); - - for (const [pattern, { owners, files }] of groups) { - if (pattern === "*") continue; - - lines.push(`### \`${pattern}\``); - lines.push(`Files: ${files.map((f) => `\`${f}\``).join(", ")}`); - - const teams = owners.filter((o) => o.includes("/")); - const individuals = owners.filter( - (o) => !o.includes("/") && o.toLowerCase() !== authorLower - ); - - if (teams.length > 0) { - lines.push(`Teams: ${teams.map((t) => `@${t}`).join(", ")}`); - } - - if (individuals.length > 0) { - const scored = individuals - .map((o) => [o, scores[o] || 0]) - .sort((a, b) => b[1] - a[1]); - - if (scored[0][1] > 0) { - const mention = MENTION_REVIEWERS ? `@${scored[0][0]}` : scored[0][0]; - lines.push(`Suggested: ${mention}`); - const rest = scored.slice(1).map(([o]) => o); - if (rest.length > 0) { - lines.push(`Also eligible: ${fmtEligible(rest)}`); - } - } else { - lines.push(`Eligible: ${fmtEligible(individuals)}`); - } - } - lines.push(""); - } - - const starGroup = groups.get("*"); - if (starGroup) { - lines.push("### General files (require maintainer)"); - lines.push(`Files: ${starGroup.files.map((f) => `\`${f}\``).join(", ")}`); - - // Only suggest maintainers for wildcard files, since only they can approve. - const maintainerSet = new Set(maintainers.map((m) => m.toLowerCase())); - const maintainerScores = Object.entries(scores) - .filter( - ([login]) => - login.toLowerCase() !== authorLower && - maintainerSet.has(login.toLowerCase()) - ) - .sort((a, b) => b[1] - a[1]); - - if (maintainerScores.length > 0 && maintainerScores[0][1] > 0) { - const [login] = maintainerScores[0]; - const dirs = topDirs(dirScores[login]); - lines.push("Based on git history:"); - lines.push(fmtReviewer(login, dirs)); - } else { - lines.push(`Pick a maintainer from ${OWNERS_LINK}.`); - } - lines.push(""); - } - - const maintainerList = maintainers - .filter((m) => m.toLowerCase() !== authorLower) - .map((m) => (MENTION_REVIEWERS ? `@${m}` : m)) - .join(", "); - - lines.push( - `Any maintainer (${maintainerList}) can approve all areas.`, - `See ${OWNERS_LINK} for ownership rules.` - ); - - return lines.join("\n") + "\n"; -} - -async function findExistingComment(github, owner, repo, prNumber) { - const comments = await github.paginate(github.rest.issues.listComments, { - owner, - repo, - issue_number: prNumber, - }); - const match = comments.find((c) => c.body && c.body.includes(MARKER)); - return match ? match.id : null; -} - -module.exports = async ({ github, context, core }) => { - const owner = context.repo.owner; - const repo = context.repo.repo; - const prNumber = context.payload.pull_request.number; - const prAuthor = context.payload.pull_request.user.login; - - const files = await getChangedFiles(github, owner, repo, prNumber); - if (files.length === 0) { - core.info("No changed files found."); - return; - } - - const now = new Date(); - const ownersPath = path.join( - process.env.GITHUB_WORKSPACE, - ".github", - "OWNERS" - ); - - const { scores, dirScores, scoredCount } = await scoreContributors( - files, - prAuthor, - now, - github, - owner, - repo - ); - const sortedScores = Object.entries(scores).sort((a, b) => b[1] - a[1]); - const eligible = parseOwnersForFiles(files, ownersPath); - - const rulesWithTeams = parseOwnersFile(ownersPath, { includeTeams: true }); - const groups = getOwnershipGroups(files, rulesWithTeams); - const maintainers = getMaintainers(rulesWithTeams); - - let comment; - if (groups.size >= 2) { - comment = buildPerGroupComment( - groups, - scores, - dirScores, - maintainers, - prAuthor, - files.length - ); - } else { - let roundRobin = null; - if (selectReviewers(sortedScores).length === 0 && eligible.length > 0) { - roundRobin = await selectRoundRobin(github, owner, repo, eligible, prAuthor); - } - - comment = buildComment( - sortedScores, - dirScores, - files.length, - scoredCount, - eligible, - prAuthor, - roundRobin - ); - } - - core.info(comment); - - const existingId = await findExistingComment(github, owner, repo, prNumber); - if (existingId) { - await github.rest.issues.updateComment({ - owner, - repo, - comment_id: existingId, - body: comment, - }); - } else { - await github.rest.issues.createComment({ - owner, - repo, - issue_number: prNumber, - body: comment, - }); - } -}; diff --git a/.github/workflows/suggest-reviewers.yml b/.github/workflows/suggest-reviewers.yml deleted file mode 100644 index 066bc6021b..0000000000 --- a/.github/workflows/suggest-reviewers.yml +++ /dev/null @@ -1,30 +0,0 @@ -name: suggest-reviewers - -on: - pull_request: - types: [opened, ready_for_review] - -permissions: - contents: read - pull-requests: write - -jobs: - suggest-reviewers: - runs-on: - group: databricks-deco-testing-runner-group - labels: ubuntu-latest-deco - if: ${{ !github.event.pull_request.draft && !github.event.pull_request.head.repo.fork }} - - steps: - - name: Checkout repository - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - fetch-depth: 0 - - - name: Suggest reviewers - uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 - with: - retries: 3 - script: |- - const script = require('./.github/workflows/suggest-reviewers.js'); - await script({ context, github, core }); diff --git a/.github/workflows/test-owners-scripts.yml b/.github/workflows/test-owners-scripts.yml index bc09ed4347..14acbb6df0 100644 --- a/.github/workflows/test-owners-scripts.yml +++ b/.github/workflows/test-owners-scripts.yml @@ -6,7 +6,6 @@ on: - '.github/scripts/**' - '.github/workflows/maintainer-approval.js' - '.github/workflows/maintainer-approval.test.js' - - '.github/workflows/suggest-reviewers.js' - '.github/OWNERS' jobs: