From edb356bd65838a2c634ebdf404151a617b81a603 Mon Sep 17 00:00:00 2001 From: Rob Anderson Date: Thu, 3 Aug 2023 16:19:45 -0700 Subject: [PATCH 1/4] get rid of the bot user and use octokit instead of axios --- app.js | 85 +++++++++++++++++++---------------------- template.yml | 8 ---- test.js | 105 ++++++++++++++++++++++++++++++++++----------------- 3 files changed, 109 insertions(+), 89 deletions(-) diff --git a/app.js b/app.js index 9286dfc..9164b99 100644 --- a/app.js +++ b/app.js @@ -23,11 +23,12 @@ module.exports = (app) => { // Approve PR, if configured to do so if (process.env.APPROVE_PR == 'true') { console.log(`Adding review to PR`); - await axios({ - method: 'post', - url: `${context.payload.pull_request.url}/reviews`, - auth: auth, - data: { "event": "APPROVE" } + await context.octokit.rest.pulls.createReview({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + pull_number: context.payload.pull_request.number, + body: "Approved by emergency PR bot", + event: "APPROVE" }).then(response => { console.log(`Review added`); }).catch(error => { @@ -46,16 +47,13 @@ module.exports = (app) => { } let issueBody = fs.readFileSync(process.env.ISSUE_BODY_FILE, 'utf8'); issueBody = issueBody.replace('#',context.payload.pull_request.html_url); - await axios({ - method: 'post', - url: `${context.payload.repository.url}/issues`, - auth: auth, - data: { - "title": process.env.ISSUE_TITLE, - "body": issueBody, - "labels": [emergencyLabel], - ...assignees - } + await context.octokit.rest.issues.create({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + title: process.env.ISSUE_TITLE, + body: issueBody, + labels: [emergencyLabel], + ...assignees }).then(response => { console.log(`Issue created`) newIssue = response.data.html_url; @@ -69,10 +67,11 @@ module.exports = (app) => { // Merge PR, if configured to do so if (process.env.MERGE_PR == 'true') { console.log(`Merging PR`); - await axios({ - method: 'put', - url: `${context.payload.pull_request.url}/merge`, - auth: auth + await context.octokit.rest.pulls.merge({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + pull_number: context.payload.pull_request.number, + //merge_method: "squash" }).then(response => { console.log(`PR merged`); }).catch(error => { @@ -135,13 +134,11 @@ module.exports = (app) => { let errorsArray = []; // Add emergency label - await axios({ - method: 'patch', - url: context.payload.pull_request.issue_url, - auth: auth, - data: { - "labels": [emergencyLabel] - } + await context.octokit.rest.issues.addLabels({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + issue_number: context.payload.pull_request.number, + labels: [emergencyLabel] }).then(response => { console.log(`${emergencyLabel} label reapplied to PR: ${context.payload.pull_request.html_url}`); }).catch(error => { @@ -167,13 +164,11 @@ module.exports = (app) => { let errorsArray = []; // Add emergency label - await axios({ - method: 'patch', - url: context.payload.issue.url, - auth: auth, - data: { - "labels": [emergencyLabel] - } + await context.octokit.rest.issues.addLabels({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + issue_number: context.payload.pull_request.number, + labels: [emergencyLabel] }).then(response => { console.log(`${emergencyLabel} label reapplied to PR: ${context.payload.issue.html_url}`); }).catch(error => { @@ -195,13 +190,11 @@ module.exports = (app) => { if (context.payload.pull_request.body.toLocaleLowerCase().includes(process.env.TRIGGER_STRING)) { // Found the trigger string, so add the emergency label to trigger the other stuff... let errorsArray = []; - await axios({ - method: 'patch', - url: context.payload.pull_request.issue_url, - auth: auth, - data: { - "labels": [emergencyLabel] - } + await context.octokit.rest.issues.addLabels({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + issue_number: context.payload.pull_request.number, + labels: [emergencyLabel] }).then(response => { console.log(`${emergencyLabel} label applied to PR: ${context.payload.pull_request.html_url}`); newIssue = response.data.html_url; @@ -224,13 +217,11 @@ module.exports = (app) => { if (context.payload.issue.pull_request && context.payload.comment.body.toLocaleLowerCase().includes(process.env.TRIGGER_STRING)) { // This is a comment on a PR and we found the trigger string, so add the emergency label to trigger the other stuff... let errorsArray = []; - await axios({ - method: 'patch', - url: context.payload.issue.url, - auth: auth, - data: { - "labels": [emergencyLabel] - } + await context.octokit.rest.issues.addLabels({ + owner: context.payload.repository.owner.login, + repo: context.payload.repository.name, + issue_number: context.payload.pull_request.number, + labels: [emergencyLabel] }).then(response => { console.log(`${emergencyLabel} label applied to PR: ${context.payload.issue.pull_request.html_url}`); newIssue = response.data.html_url; diff --git a/template.yml b/template.yml index e7752a2..3f5a19e 100644 --- a/template.yml +++ b/template.yml @@ -12,12 +12,6 @@ Parameters: githubPrivateKey: Description: The base64 encoded private key of your GitHub app Type: String - githubUser: - Description: The user with admin access - Type: String - githubPat: - Description: The PAT of the user with admin access - Type: String emergencyLabel: Description: The label to use for emergency issues Type: String @@ -101,8 +95,6 @@ Resources: APP_ID: !Ref githubAppId WEBHOOK_SECRET: !Ref githubWebhookSecret PRIVATE_KEY: !Ref githubPrivateKey - GITHUB_USER: !Ref githubUser - GITHUB_PAT: !Ref githubPat EMERGENCY_LABEL: !Ref emergencyLabel ISSUE_TITLE: !Ref issueTitle ISSUE_BODY_FILE: !Ref issueBodyFile diff --git a/test.js b/test.js index 8ef3d03..3205eb5 100644 --- a/test.js +++ b/test.js @@ -10,6 +10,7 @@ const { } = require("@probot/adapter-aws-lambda-serverless"); const payload = { + loadName: "payload", name: "pull_request", id: "1", payload: { @@ -19,7 +20,7 @@ const payload = { }, repository: { owner: { - login: "probot", + login: "robandpdx", }, name: "superbigmono", url: "https://api.github.com/repos/robandpdx/superbigmono" @@ -34,6 +35,7 @@ const payload = { } const payloadUnlabeled = { + loadName: "payloadUnlabeled", name: "pull_request", id: "1", payload: { @@ -42,13 +44,21 @@ const payloadUnlabeled = { name: "emergency" }, pull_request: { + number: 1, issue_url: "https://api.github.com/repos/robandpdx/superbigmono/issues/1", html_url: "https://github.com/robandpdx/superbigmono/pull/1", }, + repository: { + owner: { + login: "robandpdx", + }, + name: "superbigmono" + }, }, } const payloadIssueUnlabeled = { + loadName: "payloadIssueUnlabeled", name: "issues", id: "1", payload: { @@ -60,23 +70,41 @@ const payloadIssueUnlabeled = { url: "https://api.github.com/repos/robandpdx/superbigmono/issues/1", html_url: "https://github.com/robandpdx/superbigmono/pull/1", }, + pull_request: { + number: 1, + }, + repository: { + owner: { + login: "robandpdx", + }, + name: "superbigmono" + }, }, } const payloadPrOpened = { + loadName: "payloadPrOpened", name: "pull_request", id: "1", payload: { action: "opened", pull_request: { + number: 1, body: "We need an Emergency landing - this bug is critical!", issue_url: "https://api.github.com/repos/robandpdx/superbigmono/issues/1", html_url: "https://github.com/robandpdx/superbigmono/pull/1", - } + }, + repository: { + owner: { + login: "robandpdx", + }, + name: "superbigmono" + }, } } const payloadPrComment = { + loadName: "payloadPrComment", name: "issue_comment", id: "1", payload: { @@ -89,7 +117,16 @@ const payloadPrComment = { }, comment: { body: "We need an Emergency landing - this bug is critical!" - } + }, + pull_request: { + number: 1, + }, + repository: { + owner: { + login: "robandpdx", + }, + name: "superbigmono" + }, } } @@ -390,7 +427,7 @@ test("recieves pull_request.labeled event, approve (fails), create issue, merge" await probot.receive(payload); } catch (err) { assert.equal(mock.pendingMocks(), []); - assert.equal(err.errors[0][0].message, "something awful happened"); + assert.equal(err.errors[0][0].message, "request to https://api.github.com/repos/robandpdx/superbigmono/pulls/1/reviews failed, reason: something awful happened"); assert.equal(err.errors[0].length, 1); return; } @@ -427,7 +464,7 @@ test("recieves pull_request.labeled event, approve, create issue (fails), merge" await probot.receive(payload); } catch (err) { assert.equal(mock.pendingMocks(), []); - assert.equal(err.errors[0][0].message, "something awful happened"); + assert.equal(err.errors[0][0].message, "request to https://api.github.com/repos/robandpdx/superbigmono/issues failed, reason: something awful happened"); assert.equal(err.errors[0].length, 1); return; } @@ -464,7 +501,7 @@ test("recieves pull_request.labeled event, approve, create issue, merge (fails)" await probot.receive(payload); } catch (err) { assert.equal(mock.pendingMocks(), []); - assert.equal(err.errors[0][0].message, "something awful happened"); + assert.equal(err.errors[0][0].message, "request to https://api.github.com/repos/robandpdx/superbigmono/pulls/1/merge failed, reason: something awful happened"); assert.equal(err.errors[0].length, 1); return; } @@ -475,9 +512,9 @@ test("recieves pull_request.unlabeled event, reapply emergency label", async fun process.env.EMERGENCY_LABEL_PERMANENT = 'true'; // mock the request to reapply the emergency label const mock = nock("https://api.github.com") - .patch("/repos/robandpdx/superbigmono/issues/1", + .post("/repos/robandpdx/superbigmono/issues/1/labels", (requestBody) => { - assert.equal(requestBody.labels[0], "emergency"); + assert.equal(requestBody[0], "emergency"); return true; } ) @@ -488,13 +525,13 @@ test("recieves pull_request.unlabeled event, reapply emergency label", async fun }); // This test will fail to reapply the emergency label to a PR -test("recieves pull_request.unlabeled event, reapply emergency label", async function () { +test("recieves pull_request.unlabeled event, fail to reapply emergency label", async function () { process.env.EMERGENCY_LABEL_PERMANENT = 'true'; // mock the request to reapply the emergency label const mock = nock("https://api.github.com") - .patch("/repos/robandpdx/superbigmono/issues/1", + .post("/repos/robandpdx/superbigmono/issues/1/labels", (requestBody) => { - assert.equal(requestBody.labels[0], "emergency"); + assert.equal(requestBody[0], "emergency"); return true; } ) @@ -504,7 +541,7 @@ test("recieves pull_request.unlabeled event, reapply emergency label", async fun await probot.receive(payloadUnlabeled); } catch (err) { assert.equal(mock.pendingMocks(), []); - assert.equal(err.errors[0][0].message, "something awful happened"); + assert.equal(err.errors[0][0].message, "request to https://api.github.com/repos/robandpdx/superbigmono/issues/1/labels failed, reason: something awful happened"); assert.equal(err.errors[0].length, 1); return; } @@ -519,13 +556,13 @@ test("recieves pull_request.unlabeled event, dont't emergency label", async func }); // This test will reapply the emergency label to an issue -test("recieves pull_request.unlabeled event, reapply emergency label", async function () { +test("recieves issue.unlabeled event, reapply emergency label", async function () { process.env.EMERGENCY_LABEL_PERMANENT = 'true'; // mock the request to reapply the emergency label const mock = nock("https://api.github.com") - .patch("/repos/robandpdx/superbigmono/issues/1", + .post("/repos/robandpdx/superbigmono/issues/1/labels", (requestBody) => { - assert.equal(requestBody.labels[0], "emergency"); + assert.equal(requestBody[0], "emergency"); return true; } ) @@ -536,13 +573,13 @@ test("recieves pull_request.unlabeled event, reapply emergency label", async fun }); // This test will fail to reapply the emergency label to an issue -test("recieves pull_request.unlabeled event, reapply emergency label", async function () { +test("recieves issue.unlabeled event, fail to reapply emergency label", async function () { process.env.EMERGENCY_LABEL_PERMANENT = 'true'; // mock the request to reapply the emergency label const mock = nock("https://api.github.com") - .patch("/repos/robandpdx/superbigmono/issues/1", + .post("/repos/robandpdx/superbigmono/issues/1/labels", (requestBody) => { - assert.equal(requestBody.labels[0], "emergency"); + assert.equal(requestBody[0], "emergency"); return true; } ) @@ -552,7 +589,7 @@ test("recieves pull_request.unlabeled event, reapply emergency label", async fun await probot.receive(payloadIssueUnlabeled); } catch (err) { assert.equal(mock.pendingMocks(), []); - assert.equal(err.errors[0][0].message, "something awful happened"); + assert.equal(err.errors[0][0].message, "request to https://api.github.com/repos/robandpdx/superbigmono/issues/1/labels failed, reason: something awful happened"); assert.equal(err.errors[0].length, 1); return; } @@ -570,9 +607,9 @@ test("recieves pull_request.unlabeled event, dont't emergency label", async func test("recieves pull_request.opened event, applies emergency label", async function () { // mock the request to apply the emergency label const mock = nock("https://api.github.com") - .patch("/repos/robandpdx/superbigmono/issues/1", + .post("/repos/robandpdx/superbigmono/issues/1/labels", (requestBody) => { - assert.equal(requestBody.labels[0], "emergency"); + assert.equal(requestBody[0], "emergency"); return true; } ) @@ -586,9 +623,9 @@ test("recieves pull_request.opened event, applies emergency label", async functi test("recieves pull_request.opened event, fails to apply emergency label", async function () { // mock the request to apply the emergency label const mock = nock("https://api.github.com") - .patch("/repos/robandpdx/superbigmono/issues/1", + .post("/repos/robandpdx/superbigmono/issues/1/labels", (requestBody) => { - assert.equal(requestBody.labels[0], "emergency"); + assert.equal(requestBody[0], "emergency"); return true; } ) @@ -598,7 +635,7 @@ test("recieves pull_request.opened event, fails to apply emergency label", async await probot.receive(payloadPrOpened); } catch (err) { assert.equal(mock.pendingMocks(), []); - assert.equal(err.errors[0][0].message, "something awful happened"); + assert.equal(err.errors[0][0].message, "request to https://api.github.com/repos/robandpdx/superbigmono/issues/1/labels failed, reason: something awful happened"); assert.equal(err.errors[0].length, 1); return; } @@ -610,13 +647,13 @@ test("recieves pull_request.unlabeled event, dont't emergency label", async func await probot.receive(payloadPrOpened); }); -// This test will apply the emrgency label based on the contents of a comment on the PR -test("recieves pull_request.opened event, applies emergency label", async function () { +// This test will apply the emergency label based on the contents of a comment on the PR +test("recieves issue_comment.created event, applies emergency label", async function () { // mock the request to apply the emergency label const mock = nock("https://api.github.com") - .patch("/repos/robandpdx/superbigmono/issues/1", + .post("/repos/robandpdx/superbigmono/issues/1/labels", (requestBody) => { - assert.equal(requestBody.labels[0], "emergency"); + assert.equal(requestBody[0], "emergency"); return true; } ) @@ -626,13 +663,13 @@ test("recieves pull_request.opened event, applies emergency label", async functi assert.equal(mock.pendingMocks(), []); }); -// This test will fail to apply the emrgency label based on the contents of a comment on the PR -test("recieves pull_request.opened event, failes to apply emergency label", async function () { +// This test will fail to apply the emergency label based on the contents of a comment on the PR +test("recieves issue_comment.created event, failes to apply emergency label", async function () { // mock the request to apply the emergency label const mock = nock("https://api.github.com") - .patch("/repos/robandpdx/superbigmono/issues/1", + .post("/repos/robandpdx/superbigmono/issues/1/labels", (requestBody) => { - assert.equal(requestBody.labels[0], "emergency"); + assert.equal(requestBody[0], "emergency"); return true; } ) @@ -642,14 +679,14 @@ test("recieves pull_request.opened event, failes to apply emergency label", asyn await probot.receive(payloadPrComment); } catch (err) { assert.equal(mock.pendingMocks(), []); - assert.equal(err.errors[0][0].message, "something awful happened"); + assert.equal(err.errors[0][0].message, "request to https://api.github.com/repos/robandpdx/superbigmono/issues/1/labels failed, reason: something awful happened"); assert.equal(err.errors[0].length, 1); return; } }); // This test will not reapply the emergency label because the TRIGGER_STRING is not found -test("recieves pull_request.unlabeled event, dont't emergency label", async function () { +test("recieves issue_comment.created event, dont't emergency label", async function () { delete process.env.TRIGGER_STRING; await probot.receive(payloadPrComment); }); From 84e3d5687f1a2c475f334eaaea475a36edede6cd Mon Sep 17 00:00:00 2001 From: Rob Anderson Date: Thu, 3 Aug 2023 16:23:35 -0700 Subject: [PATCH 2/4] update documentation, remove bot user, allow app to bypass branch protection --- README.md | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index d5f69a0..751185a 100644 --- a/README.md +++ b/README.md @@ -22,12 +22,6 @@ The slack notification can be configured by setting the `SLACK_MESSAGE_FILE`. Th - `#l` is replaced with the label configured in `EMERGENCY_LABEL` - `#i` is replaced with the url to the issue created, if issue creation is enabled ## Environment setup -### Create Bot User -If you want the app to merge the emergency PR, and you have "Require status checks to pass before merging" in your branch protection, you will need to give the bot user `owner` permissions on the Org or repo. - -If you do not have "Require status checks to pass before merging" you can make the bot user a standard user. If you also have more than 1 approval required you will need to configure the bot user in your branch protection section "Allow specified actors to bypass pull request requirements" in order to allow merging. - -Generate a PAT for the bot user with repo scope. Configure SSO for the PAT, authorizing the PAT for your org. ### Create GitHub App Create the GH App in your org or repo. Define a client secrent. Generate a private key. #### Grant repository permissions @@ -38,6 +32,9 @@ Check `Issues` Check `Issue comment` Check `Pull request` +#### Allow app to bypass branch protection +If you want the app to merge the emergency PR, and you have "Require status checks to pass before merging" in your branch protection, you will need to allow the app to bypass branch protection. + Once you have the bot user setup and the GitHub app configured you are ready to deploy! ### Create a Slack App From 65facd5b4900b743426e7f9f8ba66b1575491650 Mon Sep 17 00:00:00 2001 From: Rob Anderson Date: Thu, 3 Aug 2023 21:19:26 -0700 Subject: [PATCH 3/4] Document additional permissions needed for app --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 751185a..f8662f4 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,7 @@ The slack notification can be configured by setting the `SLACK_MESSAGE_FILE`. Th ### Create GitHub App Create the GH App in your org or repo. Define a client secrent. Generate a private key. #### Grant repository permissions +Set Contents access to `Read & write` Set Issues access to `Read & write` Set Pull Requests access to `Read & write` #### Subscripe to events From dd939034c379572090045eb5ba2970c0fba94e78 Mon Sep 17 00:00:00 2001 From: Rob Anderson Date: Thu, 3 Aug 2023 21:58:46 -0700 Subject: [PATCH 4/4] Update doc with additional branch protection settings --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index f8662f4..64062bb 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,9 @@ Check `Issue comment` Check `Pull request` #### Allow app to bypass branch protection -If you want the app to merge the emergency PR, and you have "Require status checks to pass before merging" in your branch protection, you will need to allow the app to bypass branch protection. +If you want the app to merge the emergency PR, and have configured "Require status checks to pass before merging" in your branch protection rule, you will need to allow the app to bypass branch protection. + +If you want the app to merge the emergency PR, and have configured "Restrict who can push to matching branches" in your branch projection rule, you will need to allow the app push access to the matching branches. Once you have the bot user setup and the GitHub app configured you are ready to deploy!