From a93336d34d47ce1d85cf9686ec4f9f7ce45b6c9b Mon Sep 17 00:00:00 2001 From: Alex Mendes Date: Tue, 20 Oct 2020 21:08:19 +0100 Subject: [PATCH 1/3] fix: use fallbacks to get PR id for CodeBuild --- source/ci_source/providers/CodeBuild.ts | 46 ++++++++-- .../providers/_tests/_codebuild.test.ts | 84 ++++++++++++++----- 2 files changed, 101 insertions(+), 29 deletions(-) diff --git a/source/ci_source/providers/CodeBuild.ts b/source/ci_source/providers/CodeBuild.ts index 68c6d111b..b3419ba9f 100644 --- a/source/ci_source/providers/CodeBuild.ts +++ b/source/ci_source/providers/CodeBuild.ts @@ -1,5 +1,5 @@ import { Env, CISource } from "../ci_source" -import { ensureEnvKeysExist } from "../ci_source_helpers" +import { ensureEnvKeysExist, getPullRequestIDForBranch } from "../ci_source_helpers" /** * CI Setup @@ -10,10 +10,24 @@ import { ensureEnvKeysExist } from "../ci_source_helpers" * * Add your `DANGER_GITHUB_API_TOKEN` to your project. Edit -> Environment -> Additional configuration -> Create a parameter * + * Note that currently, there seems to be no totally reliable way to get the branch + * name from CodeBuild. Sometimes `CODEBUILD_SOURCE_VERSION` contains the + * PR number in the format pr/123, but not always. Other times it may contain + * a commit hash. `CODEBUILD_WEBHOOK_TRIGGER` will contain the pr number on the + * same format, but only for the first event, for subsequent events it should + * contain the branch number in the format branch/my-branch. So here we attempt + * to determine the PR number from one of the environment variables and if + * unsuccessful fall back to calling the API to find the PR for the branch. */ export class CodeBuild implements CISource { + private default = { prID: "0" } constructor(private readonly env: Env) {} + async setup(): Promise { + const prID = await this._getPrId() + this.default.prID = prID.toString() + } + get name(): string { return "CodeBuild" } @@ -23,12 +37,12 @@ export class CodeBuild implements CISource { } get isPR(): boolean { - const mustHave = ["CODEBUILD_BUILD_ID", "CODEBUILD_SOURCE_VERSION", "CODEBUILD_SOURCE_REPO_URL"] + const mustHave = ["CODEBUILD_BUILD_ID", "CODEBUILD_SOURCE_REPO_URL"] return ensureEnvKeysExist(this.env, mustHave) && this._isPRRequest() } get pullRequestID(): string { - return this.env.CODEBUILD_SOURCE_VERSION.split("/")[1] + return this.default.prID } get repoSlug(): string { @@ -40,9 +54,7 @@ export class CodeBuild implements CISource { } private _isPRRequest(): boolean { - const isPRSource = this.env.CODEBUILD_SOURCE_VERSION.split("/")[0] === "pr" ? true : false - const isPRIdInt = !isNaN(parseInt(this.env.CODEBUILD_SOURCE_VERSION.split("/")[1])) - return isPRSource && isPRIdInt + return this.default.prID !== "0" } private _prParseUrl(): string { @@ -51,4 +63,26 @@ export class CodeBuild implements CISource { const matches = prUrl.match(regexp) return matches ? matches[2] : "" } + + private async _getPrId(): Promise { + const sourceParts = (this.env.CODEBUILD_SOURCE_VERSION || "").split("/") + const triggerParts = (this.env.CODEBUILD_WEBHOOK_TRIGGER || "").split("/") + + const branchName = triggerParts[0] === "branch" ? triggerParts[1] : null + let prId = sourceParts[0] === "pr" ? sourceParts[1] : null + + if (!prId) { + prId = triggerParts[0] === "pr" ? triggerParts[1] : null + } + + if (!prId && branchName) { + prId = await getPullRequestIDForBranch(this, this.env, branchName) + } + + if (isNaN(parseInt(prId))) { + return "0" + } + + return prId + } } diff --git a/source/ci_source/providers/_tests/_codebuild.test.ts b/source/ci_source/providers/_tests/_codebuild.test.ts index f09992918..b8e350931 100644 --- a/source/ci_source/providers/_tests/_codebuild.test.ts +++ b/source/ci_source/providers/_tests/_codebuild.test.ts @@ -1,5 +1,6 @@ import { CodeBuild } from "../CodeBuild" import { getCISourceForEnv } from "../../get_ci_source" +import { getPullRequestIDForBranch } from "../../ci_source_helpers" const correctEnv = { CODEBUILD_BUILD_ID: "123", @@ -8,6 +9,18 @@ const correctEnv = { DANGER_GITHUB_API_TOKEN: "xxx", } +const setupCodeBuildSource = async (env: Object) => { + const source = new CodeBuild(env) + await source.setup() + + return source +} + +jest.mock("../../ci_source_helpers", () => ({ + ...jest.requireActual("../../ci_source_helpers"), + getPullRequestIDForBranch: jest.fn(), +})) + describe("being found when looking for CI", () => { it("finds CodeBuild with the right ENV", () => { const cb = getCISourceForEnv(correctEnv) @@ -16,57 +29,82 @@ describe("being found when looking for CI", () => { }) describe(".isCI", () => { - it("validates when all CodeBuild environment vars are set", () => { - const codebuild = new CodeBuild(correctEnv) + it("validates when all CodeBuild environment vars are set", async () => { + const codebuild = await setupCodeBuildSource(correctEnv) expect(codebuild.isCI).toBeTruthy() }) - it("does not validate without env", () => { - const codebuild = new CodeBuild({}) + it("does not validate without env", async () => { + const codebuild = await setupCodeBuildSource({}) expect(codebuild.isCI).toBeFalsy() }) }) describe(".isPR", () => { - it("validates when all CodeBuild environment vars are set", () => { - const codebuild = new CodeBuild(correctEnv) + it("validates when all CodeBuild environment vars are set", async () => { + const codebuild = await setupCodeBuildSource(correctEnv) expect(codebuild.isPR).toBeTruthy() }) - it("does not validate outside of CodeBuild", () => { - const codebuild = new CodeBuild({}) + it("does not validate outside of CodeBuild", async () => { + const codebuild = await setupCodeBuildSource({}) expect(codebuild.isPR).toBeFalsy() }) - const envs = ["CODEBUILD_BUILD_ID", "CODEBUILD_SOURCE_VERSION", "CODEBUILD_SOURCE_REPO_URL"] - envs.forEach((key: string) => { - let env = Object.assign({}, correctEnv) - env[key] = null - - it(`does not validate when ${key} is missing`, () => { - const codebuild = new CodeBuild({}) - expect(codebuild.isPR).toBeFalsy() - }) + it.each(["CODEBUILD_BUILD_ID", "CODEBUILD_SOURCE_REPO_URL"])(`does not validate when %s is missing`, async key => { + const copiedEnv = { ...correctEnv } + delete copiedEnv[key] + const codebuild = await setupCodeBuildSource(copiedEnv) + expect(codebuild.isPR).toBeFalsy() }) - it("needs to have a PR number", () => { + it("needs to have a PR number", async () => { let env = Object.assign({}, correctEnv) env.CODEBUILD_SOURCE_VERSION = "pr/abc" - const codebuild = new CodeBuild(env) + const codebuild = await setupCodeBuildSource(env) expect(codebuild.isPR).toBeFalsy() }) }) describe(".pullRequestID", () => { - it("splits it from the env", () => { - const codebuild = new CodeBuild({ CODEBUILD_SOURCE_VERSION: "pr/2" }) + beforeEach(() => { + jest.resetAllMocks() + }) + + it.each(["CODEBUILD_SOURCE_VERSION", "CODEBUILD_WEBHOOK_TRIGGER"])("splits it from %s", async key => { + const codebuild = await setupCodeBuildSource({ [key]: "pr/2" }) + await codebuild.setup() expect(codebuild.pullRequestID).toEqual("2") }) + + it("calls the API to get the PR number if not available in the env vars", async () => { + ;(getPullRequestIDForBranch as jest.Mock).mockResolvedValue(1) + const env = { + CODEBUILD_SOURCE_REPO_URL: "https://github.com/sharkysharks/some-repo", + CODEBUILD_WEBHOOK_TRIGGER: "branch/my-branch", + DANGER_GITHUB_API_TOKEN: "xxx", + } + const codebuild = await setupCodeBuildSource(env) + expect(codebuild.pullRequestID).toBe("1") + expect(getPullRequestIDForBranch).toHaveBeenCalledTimes(1) + expect(getPullRequestIDForBranch).toHaveBeenCalledWith(codebuild, env, "my-branch") + }) + + it("does not call the API if no PR number or branch name available in the env vars", async () => { + const env = { + CODEBUILD_SOURCE_REPO_URL: "https://github.com/sharkysharks/some-repo", + CODEBUILD_WEBHOOK_TRIGGER: "tag/my-tag", + DANGER_GITHUB_API_TOKEN: "xxx", + } + const codebuild = await setupCodeBuildSource(env) + expect(codebuild.pullRequestID).toBe("0") + expect(getPullRequestIDForBranch).not.toHaveBeenCalled() + }) }) describe(".repoSlug", () => { - it("parses it from the repo url", () => { - const codebuild = new CodeBuild(correctEnv) + it("parses it from the repo url", async () => { + const codebuild = await setupCodeBuildSource(correctEnv) expect(codebuild.repoSlug).toEqual("sharkysharks/some-repo") }) }) From 703534aa4ff1376f3567b6e975fb2594f6054f87 Mon Sep 17 00:00:00 2001 From: Alex Mendes Date: Tue, 20 Oct 2020 21:28:00 +0100 Subject: [PATCH 2/3] docs: update changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40e860b7b..145b74d2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ -- Bitbucket Cloud: Fix bug when Danger updating inline comment with summary comment. - [@hellocore] +- Fall back to alternative methods for establishing the PR number from CodeBuild - [@alexandermendes] @@ -1845,3 +1845,4 @@ Not usable for others, only stubs of classes etc. - [@orta] [@soyn]: https://github.com/Soyn [@tim3trick]: https://github.com/tim3trick [@doniyor2109]: https://github.com/doniyor2109 +[@alexandermendes]: https://github.com/alexandermendes From 061bf1a1110306006a27cc3b2900229e6af40974 Mon Sep 17 00:00:00 2001 From: Alex Mendes Date: Thu, 29 Oct 2020 01:07:48 +0000 Subject: [PATCH 3/3] test: fix async test assertions --- .../bitbucket_server/_tests/_bitbucket_server_git.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/platforms/bitbucket_server/_tests/_bitbucket_server_git.test.ts b/source/platforms/bitbucket_server/_tests/_bitbucket_server_git.test.ts index 0cb5519c7..a57ea0d60 100644 --- a/source/platforms/bitbucket_server/_tests/_bitbucket_server_git.test.ts +++ b/source/platforms/bitbucket_server/_tests/_bitbucket_server_git.test.ts @@ -129,12 +129,12 @@ describe("the dangerfile gitDSL - BitBucket Server", () => { it("checks promise rejection for line not in the diff for inline comment", async () => { const promise = bbs.findTypeOfLine(gitDSL, 2, "banana") - await expect(promise).rejects + await expect(promise).rejects.toBeUndefined() }) it("checks promise rejection for `del` line for inline comment for deleted file", async () => { const promise = bbs.findTypeOfLine(gitDSL, 0, "jest.eslint.config.js") - await expect(promise).rejects + await expect(promise).rejects.toBeUndefined() }) it("writes a JSON DSL fixture", async () => {