diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index 13f4c6bfb6..7cf7cb4c46 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -258,32 +258,6 @@ async function selectRoundRobin(github, owner, repo, eligibleOwners, prAuthor) { // --- 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", ""]; @@ -392,20 +366,56 @@ function buildSingleDomainPendingComment(sortedScores, dirScores, scoredCount, e const LEGACY_MARKER = ""; -async function postComment(github, owner, repo, prNumber, comment) { +/** + * Delete all marker and legacy marker comments from the PR. + * Used on success paths to clean up stale pending comments. + */ +async function deleteMarkerComments(github, owner, repo, prNumber) { const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: prNumber, }); - const toDelete = comments.filter(c => + for (const c of comments) { + if (c.body && (c.body.includes(MARKER) || c.body.includes(LEGACY_MARKER))) { + await github.rest.issues.deleteComment({ + owner, repo, comment_id: c.id, + }); + } + } +} + +/** + * Create or edit the marker comment. Skips the edit if the body is unchanged. + * Cleans up duplicate or legacy marker comments, keeping only the first one. + */ +async function upsertComment(github, owner, repo, prNumber, newBody) { + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: prNumber, + }); + const markerComments = 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, + + if (markerComments.length > 0) { + const existing = markerComments[0]; + + // Clean up duplicates (legacy or accidental), keep the first. + for (const c of markerComments.slice(1)) { + await github.rest.issues.deleteComment({ + owner, repo, comment_id: c.id, + }); + } + + // Skip if body is unchanged. + if (existing.body === newBody) return; + + await github.rest.issues.updateComment({ + owner, repo, comment_id: existing.id, body: newBody, }); + return; } + await github.rest.issues.createComment({ - owner, repo, issue_number: prNumber, body: comment, + owner, repo, issue_number: prNumber, body: newBody, }); } @@ -459,8 +469,7 @@ module.exports = async ({ github, context, core }) => { state: "success", description: `Approved by @${approver}`, }); - await postComment(github, owner, repo, prNumber, - buildApprovedComment(`Approved by @${approver}`)); + await deleteMarkerComments(github, owner, repo, prNumber); return; } @@ -477,8 +486,7 @@ module.exports = async ({ github, context, core }) => { state: "success", description: "Approved (maintainer-authored PR)", }); - await postComment(github, owner, repo, prNumber, - buildApprovedComment("Approved (maintainer-authored PR)")); + await deleteMarkerComments(github, owner, repo, prNumber); return; } } @@ -506,7 +514,7 @@ module.exports = async ({ github, context, core }) => { core ); - // Set commit status + // Set commit status. Approved PRs return early (commit status is sufficient). if (result.allCovered && approverLogins.length > 0) { core.info("All ownership groups have per-path approval."); await github.rest.repos.createCommitStatus({ @@ -514,7 +522,11 @@ module.exports = async ({ github, context, core }) => { state: "success", description: "All ownership groups approved", }); - } else if (result.hasWildcardFiles) { + await deleteMarkerComments(github, owner, repo, prNumber); + return; + } + + if (result.hasWildcardFiles) { const fileList = result.wildcardFiles.join(", "); const msg = `Files need maintainer review: ${fileList}. ` + @@ -561,13 +573,11 @@ module.exports = async ({ github, context, core }) => { ); const sortedScores = Object.entries(scores).sort((a, b) => b[1] - a[1]); - // Build comment based on approval state and ownership groups + // Build pending comment with reviewer suggestions. let comment; const groups = result.groups; - if (result.allCovered && approverLogins.length > 0) { - comment = buildAllGroupsApprovedComment(groups, result.approvedBy); - } else if (groups.size >= 2) { + if (groups.size >= 2) { comment = buildPendingPerGroupComment( groups, scores, dirScores, result.approvedBy, maintainers, authorLogin ); @@ -583,5 +593,5 @@ module.exports = async ({ github, context, core }) => { } core.info(comment); - await postComment(github, owner, repo, prNumber, comment); + await upsertComment(github, owner, repo, prNumber, comment); }; diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js index 3c38ddc10d..24a90c46af 100644 --- a/.github/workflows/maintainer-approval.test.js +++ b/.github/workflows/maintainer-approval.test.js @@ -62,6 +62,7 @@ function makeGithub({ reviews = [], files = [], teamMembers = {}, existingCommen const listComments = Symbol("listComments"); const statuses = []; const createdComments = []; + const updatedComments = []; const deletedCommentIds = []; const github = { @@ -89,6 +90,9 @@ function makeGithub({ reviews = [], files = [], teamMembers = {}, existingCommen createComment: async (params) => { createdComments.push(params); }, + updateComment: async (params) => { + updatedComments.push(params); + }, }, teams: { getMembershipForUserInOrg: async ({ team_slug, username }) => { @@ -103,6 +107,7 @@ function makeGithub({ reviews = [], files = [], teamMembers = {}, existingCommen }, _statuses: statuses, _comments: createdComments, + _updatedComments: updatedComments, _deletedCommentIds: deletedCommentIds, }; return github; @@ -129,7 +134,7 @@ describe("maintainer-approval", () => { fs.rmSync(tmpDir, { recursive: true }); }); - it("maintainer approved -> success", async () => { + it("maintainer approved -> success, no comment", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "maintainer1" } }, @@ -144,9 +149,32 @@ describe("maintainer-approval", () => { assert.equal(github._statuses.length, 1); assert.equal(github._statuses[0].state, "success"); assert.ok(github._statuses[0].description.includes("maintainer1")); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); }); - it("maintainer-authored PR with any approval -> success", async () => { + it("approval cleans up stale pending comment", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "maintainer1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + existingComments: [ + { id: 500, body: "\n## Waiting for approval\n..." }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses[0].state, "success"); + assert.deepEqual(github._deletedCommentIds, [500]); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); + }); + + it("maintainer-authored PR with any approval -> success, no comment", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "randomreviewer" } }, @@ -161,9 +189,11 @@ describe("maintainer-approval", () => { assert.equal(github._statuses.length, 1); assert.equal(github._statuses[0].state, "success"); assert.ok(github._statuses[0].description.includes("maintainer-authored")); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); }); - it("single domain, owner approved -> success", async () => { + it("single domain, owner approved -> success, no comment", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "jefferycheng1" } }, @@ -180,9 +210,11 @@ describe("maintainer-approval", () => { assert.equal(github._statuses.length, 1); assert.equal(github._statuses[0].state, "success"); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); }); - it("cross-domain, both approved -> success", async () => { + it("cross-domain, both approved -> success, no comment", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "jefferycheng1" } }, @@ -200,6 +232,8 @@ describe("maintainer-approval", () => { assert.equal(github._statuses.length, 1); assert.equal(github._statuses[0].state, "success"); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); }); it("cross-domain, one missing -> pending", async () => { @@ -337,9 +371,9 @@ describe("maintainer-approval", () => { fs.rmSync(noWildcardDir, { recursive: true }); }); - // --- Comment posting tests --- + // --- Comment upsert tests --- - it("posts a comment with MARKER on every run", async () => { + it("creates comment with MARKER when none exists", async () => { const github = makeGithub({ reviews: [], files: [{ filename: "cmd/pipelines/foo.go" }], @@ -351,31 +385,71 @@ describe("maintainer-approval", () => { assert.equal(github._comments.length, 1); assert.ok(github._comments[0].body.includes("")); + assert.equal(github._updatedComments.length, 0); + assert.equal(github._deletedCommentIds.length, 0); }); - it("maintainer approval posts simple approved comment", async () => { + it("edits existing comment in place when body changes", async () => { const github = makeGithub({ - reviews: [ - { state: "APPROVED", user: { login: "maintainer1" } }, - ], + 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._comments.length, 1); - assert.ok(github._comments[0].body.includes("Approved by @maintainer1")); - assert.ok(github._comments[0].body.includes("")); + assert.equal(github._updatedComments.length, 1); + assert.equal(github._updatedComments[0].comment_id, 999); + assert.ok(github._updatedComments[0].body.includes("")); + assert.equal(github._comments.length, 0); + assert.equal(github._deletedCommentIds.length, 0); + }); + + it("skips edit when comment body is unchanged", async () => { + // Stub Math.random so selectRoundRobin is deterministic across runs. + const origRandom = Math.random; + Math.random = () => 0.5; + try { + // First, run once to capture the comment body. + const github1 = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core1 = makeCore(); + await runModule({ github: github1, context: makeContext(), core: core1 }); + const expectedBody = github1._comments[0].body; + + // Second run with that body as the existing comment. + const github2 = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + existingComments: [ + { id: 100, body: expectedBody }, + ], + }); + const core2 = makeCore(); + await runModule({ github: github2, context: makeContext(), core: core2 }); + + assert.equal(github2._comments.length, 0); + assert.equal(github2._updatedComments.length, 0); + assert.equal(github2._deletedCommentIds.length, 0); + } finally { + Math.random = origRandom; + } }); - it("deletes existing comment before posting new one", async () => { + it("cleans up duplicate marker comments, keeps the first", async () => { const github = makeGithub({ reviews: [], files: [{ filename: "cmd/pipelines/foo.go" }], existingComments: [ - { id: 999, body: "\nOld comment" }, + { id: 100, body: "\nFirst" }, + { id: 200, body: "\nDuplicate" }, + { id: 300, body: "\nLegacy" }, ], }); const core = makeCore(); @@ -383,13 +457,14 @@ describe("maintainer-approval", () => { 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("")); + // Duplicates deleted, first one edited. + assert.deepEqual(github._deletedCommentIds.sort(), [200, 300]); + assert.equal(github._updatedComments.length, 1); + assert.equal(github._updatedComments[0].comment_id, 100); + assert.equal(github._comments.length, 0); }); - it("does not delete comments without the marker", async () => { + it("does not touch comments without the marker", async () => { const github = makeGithub({ reviews: [], files: [{ filename: "cmd/pipelines/foo.go" }], @@ -403,6 +478,7 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); assert.equal(github._deletedCommentIds.length, 0); + assert.equal(github._updatedComments.length, 0); assert.equal(github._comments.length, 1); }); @@ -443,27 +519,4 @@ describe("maintainer-approval", () => { 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")); - }); });