diff --git a/torchci/lib/bot/pytorchBotHandler.ts b/torchci/lib/bot/pytorchBotHandler.ts index 0ad1387f3b..4921d32ce9 100644 --- a/torchci/lib/bot/pytorchBotHandler.ts +++ b/torchci/lib/bot/pytorchBotHandler.ts @@ -11,7 +11,8 @@ import { addLabels, hasWritePermissions as _hasWP, reactOnComment, - hasWorkflowRunningPermissions as _hasWRP, + hasApprovedPullRuns, + isFirstTimeContributor, } from "./utils"; export const CIFLOW_TRUNK_LABEL = "ciflow/trunk"; @@ -43,6 +44,7 @@ class PytorchBotHandler { commentId: number; login: string; commentBody: string; + headSha: string | undefined; forceMergeMessagePat = new RegExp("^\\s*\\S+\\s+\\S+.*"); @@ -266,7 +268,7 @@ The explanation needs to be clear on why this is needed. Here are some good exam if (ignore_current) { if ( - !(await this.hasWorkflowRunningPermissions( + !(await this.hasWritePermissions( this.ctx.payload?.comment?.user?.login )) ) { @@ -283,9 +285,7 @@ The explanation needs to be clear on why this is needed. Here are some good exam if ( rebase && - !(await this.hasWorkflowRunningPermissions( - this.ctx.payload?.comment?.user?.login - )) + !(await this.hasRebasePermissions(this.ctx.payload?.comment?.user?.login)) ) { await this.addComment( "You don't have permissions to rebase this PR since you are a first time contributor. If you think this is a mistake, please contact PyTorch Dev Infra." @@ -303,14 +303,22 @@ The explanation needs to be clear on why this is needed. Here are some good exam (e: any) => e["name"] ); } - const pr_body = - JSON.stringify(this.ctx.payload?.pull_request?.body) || ""; if ( labels !== undefined && - !labels.find((x) => x === CIFLOW_TRUNK_LABEL) && - // skip applying label to codev diffs - !pr_body.includes("Differential Revision: D") + !labels.find((x) => x === CIFLOW_TRUNK_LABEL) ) { + if ( + !(await this.hasWorkflowRunningPermissions( + this.ctx.payload?.issue?.user?.login + )) + ) { + await this.addComment( + "The author doesn't have permissions to run the required trunk workflow since they do not have approvals, aborting merge. " + + "Please get/give approval for the workflows before merging again. " + + "If you think this is a mistake, please contact PyTorch Dev Infra." + ); + return; + } await addLabels(this.ctx, [CIFLOW_TRUNK_LABEL]); } } @@ -332,11 +340,7 @@ The explanation needs to be clear on why this is needed. Here are some good exam async handleRebase(branch: string) { await this.logger.log("rebase", { branch }); const { ctx } = this; - if ( - await this.hasWorkflowRunningPermissions( - ctx.payload?.comment?.user?.login - ) - ) { + if (await this.hasRebasePermissions(ctx.payload?.comment?.user?.login)) { await this.dispatchEvent("try-rebase", { branch: branch }); await this.ackComment(); } else { @@ -362,9 +366,34 @@ The explanation needs to be clear on why this is needed. Here are some good exam return _hasWP(this.ctx, username); } + async hasRebasePermissions(username: string): Promise { + return ( + (await _hasWP(this.ctx, username)) || + !(await isFirstTimeContributor(this.ctx, username)) + ); + } + async hasWorkflowRunningPermissions(username: string): Promise { - return _hasWRP(this.ctx, username); + if (await _hasWP(this.ctx, username)) { + return true; + } + if (this.headSha === undefined) { + const pullRequest = await this.ctx.octokit.pulls.get({ + owner: this.owner, + repo: this.repo, + pull_number: this.prNum, + }); + this.headSha = pullRequest.data.head.sha; + } + + return await hasApprovedPullRuns( + this.ctx.octokit, + this.ctx.payload.repository.owner.login, + this.ctx.payload.repository.name, + this.headSha! + ); } + async handleClose() { if ( this.ctx.payload?.issue?.author_association == "FIRST_TIME_CONTRIBUTOR" diff --git a/torchci/lib/bot/utils.ts b/torchci/lib/bot/utils.ts index a396911163..1b0d9bfa6e 100644 --- a/torchci/lib/bot/utils.ts +++ b/torchci/lib/bot/utils.ts @@ -266,13 +266,3 @@ export async function isFirstTimeContributor( }); return commits?.data?.length === 0; } - -export async function hasWorkflowRunningPermissions( - ctx: any, - username: string -): Promise { - return ( - (await hasWritePermissions(ctx, username)) || - !(await isFirstTimeContributor(ctx, username)) - ); -} diff --git a/torchci/test/common.ts b/torchci/test/common.ts index 1b79da11f1..c1d4701c31 100644 --- a/torchci/test/common.ts +++ b/torchci/test/common.ts @@ -33,9 +33,18 @@ export function deepCopy(obj: any) { return JSON.parse(JSON.stringify(obj)); } -export function handleScope(scope: nock.Scope) { - if (!scope.isDone()) { - console.error("pending mocks: %j", scope.pendingMocks()); +export function handleScope(scope: nock.Scope | nock.Scope[]) { + function scopeIsDone(s: nock.Scope) { + if (!s.isDone()) { + console.error("pending mocks: %j", s.pendingMocks()); + } + s.done(); + } + if (Array.isArray(scope)) { + for (const s of scope) { + scopeIsDone(s); + } + } else { + scopeIsDone(scope); } - scope.done(); } diff --git a/torchci/test/labelCommands.test.ts b/torchci/test/labelCommands.test.ts index c8fda040c9..1875b4bdda 100644 --- a/torchci/test/labelCommands.test.ts +++ b/torchci/test/labelCommands.test.ts @@ -156,17 +156,8 @@ describe("label-bot", () => { const owner = event.payload.repository.owner.login; const repo = event.payload.repository.name; const pr_number = event.payload.issue.number; - const default_branch = event.payload.repository.default_branch; const scope = nock("https://api.github.com") - .get( - `/repos/${owner}/${repo}/collaborators/${event.payload.comment.user.login}/permission` - ) - .reply(200, { permission: "read" }) - .get( - `/repos/${owner}/${repo}/commits?author=${event.payload.comment.user.login}&sha=${default_branch}&per_page=1` - ) - .reply(200, []) .get(`/repos/${owner}/${repo}/labels`) .reply(200, existingRepoLabelsResponse) .post(`/repos/${owner}/${repo}/issues/${pr_number}/comments`, (body) => { @@ -176,9 +167,21 @@ describe("label-bot", () => { return true; }) .reply(200, {}); + const additionalScopes = [ + utils.mockPermissions( + `${owner}/${repo}`, + event.payload.comment.user.login, + "read" + ), + utils.mockGetPR(`${owner}/${repo}`, pr_number, { + head: { sha: "randomsha" }, + }), + utils.mockApprovedWorkflowRuns(`${owner}/${repo}`, "randomsha", false), + ]; await probot.receive(event); handleScope(scope); + handleScope(additionalScopes); }); test("label with ciflow good permissions", async () => { @@ -193,14 +196,6 @@ describe("label-bot", () => { const default_branch = event.payload.repository.default_branch; const scope = nock("https://api.github.com") - .get( - `/repos/${owner}/${repo}/collaborators/${event.payload.comment.user.login}/permission` - ) - .reply(200, { permission: "read" }) - .get( - `/repos/${owner}/${repo}/commits?author=${event.payload.comment.user.login}&sha=${default_branch}&per_page=1` - ) - .reply(200, [{}]) .get(`/repos/${owner}/${repo}/labels`) .reply(200, existingRepoLabelsResponse) .post( @@ -216,9 +211,21 @@ describe("label-bot", () => { return true; }) .reply(200, {}); + const additionalScopes = [ + utils.mockPermissions( + `${owner}/${repo}`, + event.payload.comment.user.login, + "read" + ), + utils.mockGetPR(`${owner}/${repo}`, pr_number, { + head: { sha: "randomsha" }, + }), + utils.mockApprovedWorkflowRuns(`${owner}/${repo}`, "randomsha", true), + ]; await probot.receive(event); handleScope(scope); + handleScope(additionalScopes); }); test("label with ciflow on issue should have no event", async () => { diff --git a/torchci/test/mergeCommands.test.ts b/torchci/test/mergeCommands.test.ts index 5291a4d60b..1fc12f72d0 100644 --- a/torchci/test/mergeCommands.test.ts +++ b/torchci/test/mergeCommands.test.ts @@ -110,8 +110,55 @@ describe("merge-bot", () => { .get(`/repos/${owner}/${repo}/pulls/${pr_number}/reviews`) .reply(200, requireDeepCopy("./fixtures/pull_request_reviews.json")); + const additionalScopes = [ + utils.mockPermissions( + `${owner}/${repo}`, + event.payload.issue.user.login, + "write" + ), + ]; + + await probot.receive(event); + handleScope(scope); + handleScope(additionalScopes); + }); + + test("merge command on pytorch/pytorch pull request does not trigger dispatch if no write permissions for label", async () => { + const event = requireDeepCopy("./fixtures/pull_request_comment.json"); + + event.payload.comment.body = "@pytorchbot merge"; + event.payload.repository.owner.login = "pytorch"; + event.payload.repository.name = "pytorch"; + + const owner = event.payload.repository.owner.login; + const repo = event.payload.repository.name; + const pr_number = event.payload.issue.number; + const comment_number = event.payload.comment.id; + const scope = nock("https://api.github.com") + .get(`/repos/${owner}/${repo}/pulls/${pr_number}/reviews`) + .reply(200, requireDeepCopy("./fixtures/pull_request_reviews.json")) + .post(`/repos/${owner}/${repo}/issues/${pr_number}/comments`, (body) => { + expect(JSON.stringify(body)).toContain( + `The author doesn't have permissions to run the required trunk workflow` + ); + return true; + }) + .reply(200, {}); + const additionalScopes = [ + utils.mockGetPR(`${owner}/${repo}`, pr_number, { + head: { sha: "randomsha" }, + }), + utils.mockApprovedWorkflowRuns(`${owner}/${repo}`, "randomsha", false), + utils.mockPermissions( + `${owner}/${repo}`, + event.payload.issue.user.login, + "read" + ), + ]; + await probot.receive(event); handleScope(scope); + handleScope(additionalScopes); }); test("merge command on pull request triggers dispatch and like", async () => { @@ -385,10 +432,6 @@ describe("merge-bot", () => { `/repos/${owner}/${repo}/collaborators/${event.payload.comment.user.login}/permission` ) .reply(200, { permission: "read" }) - .get( - `/repos/${owner}/${repo}/commits?author=${event.payload.comment.user.login}&sha=${default_branch}&per_page=1` - ) - .reply(200, []) .post( `/repos/${owner}/${repo}/issues/comments/${comment_number}/reactions`, (body) => { diff --git a/torchci/test/utils.ts b/torchci/test/utils.ts index 5d2d41b35e..f40282f07e 100644 --- a/torchci/test/utils.ts +++ b/torchci/test/utils.ts @@ -44,3 +44,38 @@ export function mockAccessToken(): void { .post("/app/installations/2/access_tokens") .reply(200, { token: "test" }); } + +export function mockPermissions( + repoFullName: string, + user: string, + permission: string = "write" +) { + return nock("https://api.github.com") + .get(`/repos/${repoFullName}/collaborators/${user}/permission`) + .reply(200, { + permission: permission, + }); +} + +export function mockApprovedWorkflowRuns( + repoFullname: string, + headSha: string, + approved: boolean +) { + return nock("https://api.github.com") + .get(`/repos/${repoFullname}/actions/runs?head_sha=${headSha}`) + .reply(200, { + workflow_runs: [ + { + event: "pull_request", + conclusion: approved ? "success" : "action_required", + }, + ], + }); +} + +export function mockGetPR(repoFullName: string, prNumber: number, body: any) { + return nock("https://api.github.com") + .get(`/repos/${repoFullName}/pulls/${prNumber}`) + .reply(200, body); +}