Skip to content

Commit

Permalink
[bots] Do not add ciflow/trunk label if the author does not have perm…
Browse files Browse the repository at this point in the history 
…issions (#5025)

Do not add ciflow/trunk on merge if the author has no write permission.
Instead, abort merge (it would have failed anyways) and post a comment
about seeking approvals

Change -i permission to check for write permissions instead of write
permissions OR has other commit on main

Attempts to move some common mocks to be functions so I don't need to
stare at urls as much

Helps reduce bot fighting in the below case:
<img width="884" alt="image"
src="https://github.com/pytorch/test-infra/assets/44682903/340a999a-38da-4ab5-9c7d-5ee8643bb0df">
  • Loading branch information
@clee2000
clee2000 authored Mar 19, 2024
1 parent 6e81f08 commit b659b37
Show file tree
Hide file tree
Showing 6 changed files with 164 additions and 51 deletions.
61 changes: 45 additions & 16 deletions torchci/lib/bot/pytorchBotHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import {
addLabels,
hasWritePermissions as _hasWP,
reactOnComment,
hasWorkflowRunningPermissions as _hasWRP,
hasApprovedPullRuns,
isFirstTimeContributor,
} from "./utils";

export const CIFLOW_TRUNK_LABEL = "ciflow/trunk";
Expand Down Expand Up @@ -43,6 +44,7 @@ class PytorchBotHandler {
commentId: number;
login: string;
commentBody: string;
headSha: string | undefined;

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

Expand Down Expand Up @@ -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
))
) {
Expand All @@ -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."
Expand All @@ -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]);
}
}
Expand All @@ -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 {
Expand All @@ -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<boolean> {
return (
(await _hasWP(this.ctx, username)) ||
!(await isFirstTimeContributor(this.ctx, username))
);
}

async hasWorkflowRunningPermissions(username: string): Promise<boolean> {
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"
Expand Down
10 changes: 0 additions & 10 deletions torchci/lib/bot/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,13 +266,3 @@ export async function isFirstTimeContributor(
});
return commits?.data?.length === 0;
}

export async function hasWorkflowRunningPermissions(
ctx: any,
username: string
): Promise<boolean> {
return (
(await hasWritePermissions(ctx, username)) ||
!(await isFirstTimeContributor(ctx, username))
);
}
17 changes: 13 additions & 4 deletions torchci/test/common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
41 changes: 24 additions & 17 deletions torchci/test/labelCommands.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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 () => {
Expand All @@ -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(
Expand All @@ -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 () => {
Expand Down
51 changes: 47 additions & 4 deletions torchci/test/mergeCommands.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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) => {
Expand Down
35 changes: 35 additions & 0 deletions torchci/test/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit b659b37

Please sign in to comment.