Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions torchci/lib/bot/pytorchBot.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { Probot } from "probot";
import { getInputArgs } from "./cliParser";
import PytorchBotHandler from "./pytorchBotHandler";
import { CachedConfigTracker } from "./utils";

function pytorchBot(app: Probot): void {
const cachedConfigTracker = new CachedConfigTracker(app);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any idea how long the cache is persisted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not persisted at all, but repeated calls to config methods during execution of the command would not need to make new network calls to fetch the same info.


app.on("issue_comment.created", async (ctx) => {
const commentBody = ctx.payload.comment.body;
const owner = ctx.payload.repository.owner.login;
Expand All @@ -20,6 +23,7 @@ function pytorchBot(app: Probot): void {
commentId,
commentBody,
useReactions: true,
cachedConfigTracker,
});
const is_pr_comment = ctx.payload.issue.pull_request != null;
const skipUsers = [
Expand Down Expand Up @@ -57,6 +61,7 @@ function pytorchBot(app: Probot): void {
commentId,
commentBody: reviewBody ?? "",
useReactions: false,
cachedConfigTracker,
});
if (reviewBody == null) {
return;
Expand Down
13 changes: 13 additions & 0 deletions torchci/lib/bot/pytorchBotHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { cherryPickClassifications } from "./Constants";
import PytorchBotLogger from "./pytorchbotLogger";
import {
addLabels,
CachedConfigTracker,
hasApprovedPullRuns,
hasWritePermissions as _hasWP,
isFirstTimeContributor,
Expand All @@ -27,6 +28,7 @@ export interface PytorchbotParams {
commentId: number;
commentBody: string;
useReactions: boolean;
cachedConfigTracker: CachedConfigTracker;
}

const PR_COMMENTED = "commented";
Expand All @@ -45,6 +47,7 @@ class PytorchBotHandler {
login: string;
commentBody: string;
headSha: string | undefined;
cachedConfigTracker: CachedConfigTracker;

forceMergeMessagePat = new RegExp("^\\s*\\S+\\s+\\S+.*");

Expand All @@ -60,6 +63,7 @@ class PytorchBotHandler {
this.commentId = params.commentId;
this.commentBody = params.commentBody;
this.useReactions = params.useReactions;
this.cachedConfigTracker = params.cachedConfigTracker;

this.logger = new PytorchBotLogger(params);
}
Expand Down Expand Up @@ -243,6 +247,15 @@ The explanation needs to be clear on why this is needed. Here are some good exam
rebase: string | boolean,
ic: boolean
) {
const config: any = await this.cachedConfigTracker.loadConfig(this.ctx);
if (config == null || !config["mergebot"]) {
await this.handleConfused(
true,
"Mergebot is not configured for this repository. Please use the merge button provided by GitHub."
);
return;
}

const extra_data = {
forceMessage,
rebase,
Expand Down
117 changes: 117 additions & 0 deletions torchci/test/mergeCommands.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe("merge-bot", () => {
beforeEach(() => {
probot = utils.testProbot();
probot.load(pytorchBot);
utils.mockConfig("pytorch-probot.yml", "mergebot: True");
});

afterEach(() => {
Expand Down Expand Up @@ -1499,3 +1500,119 @@ some other text lol
handleScope(scope);
});
});

describe("merge-bot not supported repo", () => {
let probot: probot.Probot;

beforeEach(() => {
probot = utils.testProbot();
probot.load(pytorchBot);
});

afterEach(() => {
nock.cleanAll();
jest.restoreAllMocks();
});

test("no config", 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 scopes = [
nock("https://api.github.com")
.get(`/repos/${owner}/${repo}/contents/.github%2Fpytorch-probot.yml`)
.reply(200, {
message: "Not Found",
documentation_url:
"https://docs.github.com/rest/repos/contents#get-repository-content",
})
.post(
`/repos/${owner}/${repo}/issues/comments/${comment_number}/reactions`,
(body) => {
expect(JSON.stringify(body)).toContain('{"content":"confused"}');
return true;
}
)
.reply(200, {}),
utils.mockPostComment(`${owner}/${repo}`, pr_number, [
"Mergebot is not configured for this repository",
]),
];

await probot.receive(event);
handleScope(scopes);
});

test("config does not have mergebot key", 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;
utils.mockConfig("pytorch-probot.yml", "hello: true", `${owner}/${repo}`);
const scopes = [
nock("https://api.github.com")
.post(
`/repos/${owner}/${repo}/issues/comments/${comment_number}/reactions`,
(body) => {
expect(JSON.stringify(body)).toContain('{"content":"confused"}');
return true;
}
)
.reply(200, {}),
utils.mockPostComment(`${owner}/${repo}`, pr_number, [
"Mergebot is not configured for this repository",
]),
];

await probot.receive(event);
handleScope(scopes);
});

test("config mergebot key set to false", 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;
utils.mockConfig(
"pytorch-probot.yml",
"mergebot: False",
`${owner}/${repo}`
);
const scopes = [
nock("https://api.github.com")
.post(
`/repos/${owner}/${repo}/issues/comments/${comment_number}/reactions`,
(body) => {
expect(JSON.stringify(body)).toContain('{"content":"confused"}');
return true;
}
)
.reply(200, {}),
utils.mockPostComment(`${owner}/${repo}`, pr_number, [
"Mergebot is not configured for this repository",
]),
];

await probot.receive(event);
handleScope(scopes);
});
});
11 changes: 9 additions & 2 deletions torchci/test/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,22 @@ export function testOctokit(): Octokit {
export function mockConfig(
fileName: string,
content: string,
repoKey: string
repoKey: string | RegExp = ".*"
): void {
const configPayload = require("./fixtures/config.json");
configPayload["content"] = Buffer.from(content).toString("base64");
configPayload["name"] = fileName;
configPayload["path"] = `.github/${fileName}`;
nock("https://api.github.com")
.get(
`/repos/${repoKey}/contents/${encodeURIComponent(`.github/${fileName}`)}`
// The use of regex here means that if the repokey or the filename contain
// regex special characters, they will be viewed as regex. The main one
// to worry about is `.` but I think it will cause minimal problems
RegExp(
`/repos/${repoKey}/contents/${encodeURIComponent(
`.github/${fileName}`
)}`
)
)
.times(2)
.reply(200, content);
Expand Down