diff --git a/.github/workflows/lib/approval.js b/.github/workflows/lib/approval.js index 6fb2b10..7548278 100644 --- a/.github/workflows/lib/approval.js +++ b/.github/workflows/lib/approval.js @@ -20,6 +20,33 @@ class ApprovalManager { return users.length ? users.map((u) => `[@${u}](https://github.com/${u})`).join(", ") : "-"; } + // Determine the status of a pipeline proposal from durable issue state and + // votes, rather than from the event that happened to trigger the workflow. + determinePipelineStatus(issue) { + const labels = (issue.labels || []).map((label) => (typeof label === "string" ? label : label.name)); + const approvalThresholdReached = + this.coreApprovals.size >= 2 || (this.coreApprovals.size >= 1 && this.maintainerApprovals.size >= 1); + const rejectionThresholdReached = + (this.coreRejections.size >= 2 || (this.coreRejections.size >= 1 && this.maintainerRejections.size >= 1)) && + this.coreApprovals.size === 0 && + this.maintainerApprovals.size === 0; + const closedAsRejected = + issue.state === "closed" && + issue.state_reason === "not_planned" && + (this.coreRejections.size > 0 || this.maintainerRejections.size > 0); + + if (labels.includes("timed-out")) { + return "⏰ Timed Out"; + } + if (closedAsRejected || rejectionThresholdReached) { + return "❌ Rejected"; + } + if (approvalThresholdReached) { + return "✅ Approved"; + } + return "🕐 Pending"; + } + // Helper to fetch team members async getTeamMembers(teamSlug) { try { @@ -91,6 +118,16 @@ class ApprovalManager { // Combine existing non-status labels with new status label const updatedLabels = [...existingLabels, newStatusLabel]; + // Avoid triggering another workflow run when the labels are already right. + const currentLabels = issue.data.labels.map((label) => (typeof label === "string" ? label : label.name)); + if ( + currentLabels.length === updatedLabels.length && + currentLabels.every((label) => updatedLabels.includes(label)) + ) { + console.log("Issue labels already up to date - no update required."); + return; + } + // Update labels await this.github.rest.issues.update({ owner: this.org, @@ -102,7 +139,21 @@ class ApprovalManager { // Helper to find and update status comment async updateStatusComment(statusBody) { - let statusComment = this.comments.find((c) => c.body.startsWith("## Approval status:")); + const statusComments = this.comments.filter((c) => c.body && c.body.startsWith("## Approval status:")); + const statusComment = statusComments[0]; + + // Opening an issue also applies its template labels. Older workflow runs + // could race and create more than one status comment; clean those up when + // the issue is next processed. + for (const duplicate of statusComments.slice(1)) { + console.log(`Deleting duplicate status comment ${duplicate.id}.`); + await this.github.rest.issues.deleteComment({ + owner: this.org, + repo: this.repo, + comment_id: duplicate.id, + }); + } + if (statusComment) { if (statusComment.body.trim() === statusBody.trim()) { console.log("Status comment already up to date - no update required."); diff --git a/.github/workflows/lib/approval.test.js b/.github/workflows/lib/approval.test.js index 98bb9d5..3fb071c 100644 --- a/.github/workflows/lib/approval.test.js +++ b/.github/workflows/lib/approval.test.js @@ -11,6 +11,7 @@ const mockGithub = { update: jest.fn(), updateComment: jest.fn(), createComment: jest.fn(), + deleteComment: jest.fn(), }, }, }; @@ -60,6 +61,56 @@ describe("ApprovalManager", () => { }); }); + describe("determinePipelineStatus", () => { + const issue = (overrides = {}) => ({ state: "open", state_reason: null, labels: [], ...overrides }); + + it("should approve with 2 core approvals", () => { + approvalManager.coreApprovals = new Set(["core1", "core2"]); + + expect(approvalManager.determinePipelineStatus(issue())).toBe("✅ Approved"); + }); + + it("should reject with 2 core rejections and no approvals", () => { + approvalManager.coreRejections = new Set(["core1", "core2"]); + + expect(approvalManager.determinePipelineStatus(issue())).toBe("❌ Rejected"); + }); + + it("should reject with 1 core and 1 maintainer rejection", () => { + approvalManager.coreRejections = new Set(["core1"]); + approvalManager.maintainerRejections = new Set(["maintainer1"]); + + expect(approvalManager.determinePipelineStatus(issue())).toBe("❌ Rejected"); + }); + + it("should stay pending when rejection votes are opposed by an approval", () => { + approvalManager.coreRejections = new Set(["core1", "core2"]); + approvalManager.coreApprovals = new Set(["core3"]); + + expect(approvalManager.determinePipelineStatus(issue())).toBe("🕐 Pending"); + }); + + it("should preserve a manual rejected closure on later events", () => { + approvalManager.coreRejections = new Set(["core1"]); + + expect(approvalManager.determinePipelineStatus(issue({ state: "closed", state_reason: "not_planned" }))).toBe( + "❌ Rejected", + ); + }); + + it("should handle the Dragen case regardless of the incorrect close reason", () => { + approvalManager.coreRejections = new Set(["core1", "core2", "core3", "core4"]); + + expect(approvalManager.determinePipelineStatus(issue({ state: "closed", state_reason: "completed" }))).toBe( + "❌ Rejected", + ); + }); + + it("should preserve the timed-out label", () => { + expect(approvalManager.determinePipelineStatus(issue({ labels: [{ name: "timed-out" }] }))).toBe("⏰ Timed Out"); + }); + }); + describe("getTeamMembers", () => { it("should fetch team members successfully", async () => { const mockMembers = [{ login: "user1" }, { login: "user2" }, { login: "user3" }]; @@ -174,6 +225,12 @@ describe("ApprovalManager", () => { }); it("should add proposed label for pending status", async () => { + mockGithub.rest.issues.get.mockResolvedValueOnce({ + data: { + labels: [{ name: "bug" }, { name: "accepted" }, { name: "enhancement" }], + }, + }); + await approvalManager.updateIssueStatus("🕐 Pending"); expect(mockGithub.rest.issues.get).toHaveBeenCalledWith({ @@ -211,6 +268,16 @@ describe("ApprovalManager", () => { labels: ["documentation", "priority-high", "turned-down"], }); }); + + it("should not update labels when they are already correct", async () => { + mockGithub.rest.issues.get.mockResolvedValueOnce({ + data: { labels: [{ name: "bug" }, { name: "accepted" }] }, + }); + + await approvalManager.updateIssueStatus("✅ Approved"); + + expect(mockGithub.rest.issues.update).not.toHaveBeenCalled(); + }); }); describe("updateStatusComment", () => { @@ -252,6 +319,42 @@ describe("ApprovalManager", () => { body: statusBody, }); }); + + it("should delete duplicate status comments and update the first", async () => { + approvalManager.comments = [ + { id: 1, body: "## Approval status: Old status" }, + { id: 2, body: "## Approval status: Old status" }, + ]; + const newStatusBody = "## Approval status: New status"; + + await approvalManager.updateStatusComment(newStatusBody); + + expect(mockGithub.rest.issues.deleteComment).toHaveBeenCalledWith({ + owner: mockOrg, + repo: mockRepo, + comment_id: 2, + }); + expect(mockGithub.rest.issues.updateComment).toHaveBeenCalledWith({ + owner: mockOrg, + repo: mockRepo, + comment_id: 1, + body: newStatusBody, + }); + }); + + it("should ignore comments with no body", async () => { + approvalManager.comments = [{ id: 1, body: null }]; + const statusBody = "## Approval status: New status"; + + await approvalManager.updateStatusComment(statusBody); + + expect(mockGithub.rest.issues.createComment).toHaveBeenCalledWith({ + owner: mockOrg, + repo: mockRepo, + issue_number: mockIssueNumber, + body: statusBody, + }); + }); }); describe("processComments", () => { diff --git a/.github/workflows/pipeline_proposals.yml b/.github/workflows/pipeline_proposals.yml index fb8e629..8598275 100644 --- a/.github/workflows/pipeline_proposals.yml +++ b/.github/workflows/pipeline_proposals.yml @@ -6,6 +6,12 @@ on: issue_comment: types: [created, edited] +# Issue templates apply labels at the same time as the issue is opened. Process +# those events in order so they cannot race to create or overwrite status state. +concurrency: + group: ${{ github.workflow }}-${{ github.event.issue.number }} + cancel-in-progress: false + jobs: pipeline_approval: # Only run for pipeline proposal issues @@ -69,37 +75,9 @@ jobs: return body; } - // Handle label changes - if (context.eventName === 'issues' && (context.payload.action === 'labeled' || context.payload.action === 'unlabeled')) { - const label = context.payload.label.name; - if (label === 'timed-out') { - console.log('Timed-out label detected, updating status'); - const statusBody = generateStatusBody('⏰ Timed Out'); - await approvalManager.updateStatusComment(statusBody); - return; - } - } - - // Handle new issue creation - if (context.eventName === 'issues' && context.payload.action === 'opened') { - const body = generateStatusBody('🕐 Pending'); - console.log('Creating initial comment for review status'); - await approvalManager.updateStatusComment(body); - await approvalManager.updateIssueStatus('🕐 Pending'); - return; - } - - - - - // Determine status - let status = '🕐 Pending'; - - if (context.eventName === 'issues' && context.payload.action === 'closed' && context.payload.issue.state_reason === 'not_planned' && (approvalManager.coreRejections.size > 0 || approvalManager.maintainerRejections.size > 0)) { - status = '❌ Rejected'; - } else if ((approvalManager.coreApprovals.size >= 2) || (approvalManager.coreApprovals.size >= 1 && approvalManager.maintainerApprovals.size >= 1)) { - status = '✅ Approved'; - } + // Derive status from the issue and all votes on every event. This + // keeps later label events from reverting a rejected issue to pending. + const status = approvalManager.determinePipelineStatus(context.payload.issue); const statusBody = generateStatusBody(status); console.log('New status body to post:\n', statusBody); diff --git a/.github/workflows/rfc_approval.yml b/.github/workflows/rfc_approval.yml index 8153ca5..10f98be 100644 --- a/.github/workflows/rfc_approval.yml +++ b/.github/workflows/rfc_approval.yml @@ -6,6 +6,10 @@ on: issue_comment: types: [created, edited] +concurrency: + group: ${{ github.workflow }}-${{ github.event.issue.number }} + cancel-in-progress: false + jobs: rfc_approval: # Only run for RFC proposal issues diff --git a/.github/workflows/sig_approval.yml b/.github/workflows/sig_approval.yml index 276e3cb..207f4a4 100644 --- a/.github/workflows/sig_approval.yml +++ b/.github/workflows/sig_approval.yml @@ -6,6 +6,10 @@ on: issue_comment: types: [created, edited] +concurrency: + group: ${{ github.workflow }}-${{ github.event.issue.number }} + cancel-in-progress: false + jobs: sig_approval: # Only run for SIG proposal issues diff --git a/.github/workflows/test-approval-automation.yml b/.github/workflows/test-approval-automation.yml index 4e0388f..5909663 100644 --- a/.github/workflows/test-approval-automation.yml +++ b/.github/workflows/test-approval-automation.yml @@ -7,6 +7,7 @@ on: - ".github/workflows/lib/**" - ".github/workflows/pipeline_proposals.yml" - ".github/workflows/rfc_approval.yml" + - ".github/workflows/sig_approval.yml" - ".github/workflows/test-approval-automation.yml" push: branches: [main, master] @@ -14,6 +15,7 @@ on: - ".github/workflows/lib/**" - ".github/workflows/pipeline_proposals.yml" - ".github/workflows/rfc_approval.yml" + - ".github/workflows/sig_approval.yml" - ".github/workflows/test-approval-automation.yml" jobs: