[bots] Bot to comment wiki link on codev PRs with no write permissions (

#5027) Comments on PRs in pytorch/pytorch that look like they were exported from phab but the author doesn't have write permissions. Changed check for Differential Revision string to also take ones that have markdown links (like `[D11111](link)`) I've seen them around, I think they're usually ghstack/stacked diff PRs
pytorch · Mar 20, 2024 · 93e5163 · 93e5163
1 parent b659b37
commit 93e5163
Show file tree

Hide file tree

Showing 6 changed files with 164 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
+}