diff --git a/torchci/lib/bot/autoLabelBot.ts b/torchci/lib/bot/autoLabelBot.ts index 7d50412b38..7666dc568a 100644 --- a/torchci/lib/bot/autoLabelBot.ts +++ b/torchci/lib/bot/autoLabelBot.ts @@ -5,9 +5,12 @@ import { hasApprovedPullRuns, hasWritePermissions, isPyTorchPyTorch, - repoKey, } from "./utils"; import { minimatch } from "minimatch"; +import { + CODEV_INDICATOR, + genCodevNoWritePermComment, +} from "./codevNoWritePermBot"; const titleRegexToLabel: [RegExp, string][] = [ [/rocm/gi, "module: rocm"], @@ -438,6 +441,7 @@ function myBot(app: Probot): void { const owner = context.payload.repository.owner.login; const repo = context.payload.repository.name; const prAuthor = context.payload.pull_request.user.login; + const body = context.payload.pull_request.body; if (context.payload.review.state !== "approved") { return; @@ -448,11 +452,7 @@ function myBot(app: Probot): void { } // only applies label to codev diffs. - if ( - !JSON.stringify(context.payload.pull_request.body).includes( - "Differential Revision: D" - ) - ) { + if (!body?.match(CODEV_INDICATOR)) { return; } @@ -468,15 +468,11 @@ function myBot(app: Probot): void { )) && !(await hasWritePermissions(context, prAuthor)) ) { - const wikiLink = - "https://www.internalfb.com/intern/wiki/PyTorch/PyTorchDev/Workflow/develop/#setup-your-github-accoun"; await context.octokit.issues.createComment({ owner, repo, issue_number: context.payload.pull_request.number, - body: - "This appears to be a codev diff but the PR author doesn't have write permissions. " + - `@${prAuthor}, please do step 2 of [internal wiki](${wikiLink}) to get write access so you do not need to get approvals in the future.`, + body: genCodevNoWritePermComment(prAuthor), }); return; } diff --git a/torchci/lib/bot/codevNoWritePermBot.ts b/torchci/lib/bot/codevNoWritePermBot.ts new file mode 100644 index 0000000000..f685c84e4c --- /dev/null +++ b/torchci/lib/bot/codevNoWritePermBot.ts @@ -0,0 +1,41 @@ +import { hasWritePermissions, isPyTorchPyTorch } from "./utils"; +import { Probot } from "probot"; + +export const CODEV_INDICATOR = /Differential Revision: \[?D/; +const CODEV_WIKI_LINK = + "https://www.internalfb.com/intern/wiki/PyTorch/PyTorchDev/Workflow/develop/#setup-your-github-accoun"; + +export function genCodevNoWritePermComment(author: string) { + return ( + "This appears to be a diff that was exported from phabricator, " + + `but the PR author does not have sufficient permissions to run CI. ` + + `@${author}, please do step 2 of [internal wiki](${CODEV_WIKI_LINK}) to get write access so ` + + `you do not need to get CI approvals in the future. ` + + "If you think this is a mistake, please contact the Pytorch Dev Infra team." + ); +} + +// If a pytorch/pytorch codev PR is created but the author doesn't have write +// permissions, the bot will comment on the PR to inform the author to get write +// access. +export default function codevNoWritePerm(app: Probot): void { + app.on("pull_request.opened", async (context) => { + const body = context.payload.pull_request.body; + const author = context.payload.pull_request.user.login; + const prNumber = context.payload.pull_request.number; + const owner = context.payload.repository.owner.login; + const repo = context.payload.repository.name; + if ( + isPyTorchPyTorch(owner, repo) && + body?.match(CODEV_INDICATOR) && + !(await hasWritePermissions(context, author)) + ) { + await context.octokit.rest.issues.createComment({ + owner, + repo, + issue_number: prNumber, + body: genCodevNoWritePermComment(author), + }); + } + }); +} diff --git a/torchci/lib/bot/index.ts b/torchci/lib/bot/index.ts index 9385c7f8f7..f269aefd26 100644 --- a/torchci/lib/bot/index.ts +++ b/torchci/lib/bot/index.ts @@ -10,6 +10,7 @@ import verifyDisableTestIssueBot from "./verifyDisableTestIssueBot"; import webhookToDynamo from "./webhookToDynamo"; import cancelWorkflowsOnCloseBot from "./cancelWorkflowsOnCloseBot"; import stripApprovalBot from "./stripApprovalBot"; +import codevNoWritePerm from "./codevNoWritePermBot"; export default function bot(app: Probot) { autoCcBot(app); @@ -23,4 +24,5 @@ export default function bot(app: Probot) { drciBot(app); retryBot(app); cancelWorkflowsOnCloseBot(app); + codevNoWritePerm(app); } diff --git a/torchci/test/autoLabelBot.test.ts b/torchci/test/autoLabelBot.test.ts index 3289f56d66..2f272cdc6d 100644 --- a/torchci/test/autoLabelBot.test.ts +++ b/torchci/test/autoLabelBot.test.ts @@ -1,7 +1,7 @@ import nock from "nock"; import { Probot } from "probot"; import * as utils from "./utils"; -import { requireDeepCopy } from "./common"; +import { handleScope, requireDeepCopy } from "./common"; import myProbotApp from "../lib/bot/autoLabelBot"; import * as botUtils from "lib/bot/utils"; @@ -861,37 +861,22 @@ describe("auto-label-bot", () => { const pr_number = event.payload.pull_request.number; const author = event.payload.pull_request.user.login; const repoFullName = `${event.payload.repository.owner.login}/${event.payload.repository.name}`; + const headSha = event.payload.pull_request.head.sha; nock("https://api.github.com") .post("/app/installations/2/access_tokens") - .reply(200, { token: "test" }) - // Nocks for not having permissions - .get((uri) => uri.startsWith(`/repos/${repoFullName}/actions/runs`)) - .reply(200, { - workflow_runs: [ - { - event: "pull_request", - conclusion: "action_required", - }, - ], - }) - .get(`/repos/${repoFullName}/collaborators/${author}/permission`) - .reply(200, { - permission: "read", - }); + .reply(200, { token: "test" }); - const scope = nock("https://api.github.com") - .post(`/repos/${repoFullName}/issues/${pr_number}/comments`, (body) => { - console.log(body); - expect(JSON.stringify(body)).toContain( - `a codev diff but the PR author doesn't have write permissions` - ); - return true; - }) - .reply(200, {}); + const scope = [ + utils.mockPermissions(repoFullName, author, "read"), + utils.mockApprovedWorkflowRuns(repoFullName, headSha, false), + utils.mockPostComment(repoFullName, pr_number, [ + "This appears to be a diff that was exported from phabricator, ", + ]), + ]; await probot.receive(event); - scope.done(); + handleScope(scope); }); }); diff --git a/torchci/test/codevNoWritePermBot.test.ts b/torchci/test/codevNoWritePermBot.test.ts new file mode 100644 index 0000000000..a8caa87a2d --- /dev/null +++ b/torchci/test/codevNoWritePermBot.test.ts @@ -0,0 +1,88 @@ +import nock from "nock"; +import * as utils from "./utils"; +import myProbotApp from "../lib/bot/codevNoWritePermBot"; +import { handleScope, requireDeepCopy } from "./common"; +import { Probot } from "probot"; +import * as botUtils from "lib/bot/utils"; + +nock.disableNetConnect(); + +describe("codevNoWritePermBot", () => { + let probot: Probot; + + beforeEach(() => { + probot = utils.testProbot(); + probot.load(myProbotApp); + nock("https://api.github.com") + .post("/app/installations/2/access_tokens") + .reply(200, { token: "test" }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + nock.cleanAll(); + }); + + function mockIsPytorchPytorch(bool: boolean) { + return jest.spyOn(botUtils, "isPyTorchPyTorch").mockReturnValue(bool); + } + + test("ignore non pytorch/pytorch PR", async () => { + const payload = requireDeepCopy("./fixtures/pull_request.opened"); + payload.payload.pull_request.body = "Differential Revision: D123123123"; + mockIsPytorchPytorch(false); + await probot.receive(payload); + }); + + test("ignore pytorch/pytorch PR that is not codev", async () => { + const payload = requireDeepCopy("./fixtures/pull_request.opened"); + mockIsPytorchPytorch(true); + payload.payload.pull_request.body = "This is not a codev PR"; + await probot.receive(payload); + }); + + test("do not comment if has write perms", async () => { + const payload = requireDeepCopy("./fixtures/pull_request.opened"); + payload.payload.pull_request.body = "Differential Revision: D123123123"; + const repoFullName = payload.payload.repository.full_name; + const author = payload.payload.pull_request.user.login; + mockIsPytorchPytorch(true); + const scope = utils.mockPermissions(repoFullName, author, "write"); + await probot.receive(payload); + handleScope(scope); + }); + + test("comment if no write perms", async () => { + const payload = requireDeepCopy("./fixtures/pull_request.opened"); + payload.payload.pull_request.body = "Differential Revision: D123123123"; + const repoFullName = payload.payload.repository.full_name; + const author = payload.payload.pull_request.user.login; + mockIsPytorchPytorch(true); + const scopes = [ + utils.mockPermissions(repoFullName, author, "read"), + utils.mockPostComment(repoFullName, 31, [ + `This appears to be a diff that was exported from phabricator`, + ]), + ]; + await probot.receive(payload); + handleScope(scopes); + }); + + test("comment if no write perms, alternate magic string", async () => { + // Same as the previous test, but with a different body + const payload = requireDeepCopy("./fixtures/pull_request.opened"); + payload.payload.pull_request.body = + "Differential Revision: [D123123123](Link)"; + const repoFullName = payload.payload.repository.full_name; + const author = payload.payload.pull_request.user.login; + mockIsPytorchPytorch(true); + const scopes = [ + utils.mockPermissions(repoFullName, author, "read"), + utils.mockPostComment(repoFullName, 31, [ + `This appears to be a diff that was exported from phabricator`, + ]), + ]; + await probot.receive(payload); + handleScope(scopes); + }); +}); diff --git a/torchci/test/utils.ts b/torchci/test/utils.ts index f40282f07e..1e006969fe 100644 --- a/torchci/test/utils.ts +++ b/torchci/test/utils.ts @@ -79,3 +79,18 @@ export function mockGetPR(repoFullName: string, prNumber: number, body: any) { .get(`/repos/${repoFullName}/pulls/${prNumber}`) .reply(200, body); } + +export function mockPostComment( + repoFullName: string, + prNumber: number, + containedStrings: string[] +) { + return nock("https://api.github.com") + .post(`/repos/${repoFullName}/issues/${prNumber}/comments`, (body) => { + for (const containedString of containedStrings) { + expect(JSON.stringify(body)).toContain(containedString); + } + return true; + }) + .reply(200); +}