diff --git a/spec/fixtures/backport_pull_request.merged.json b/spec/fixtures/backport_pull_request.merged.json index 00f857d..4124bc1 100644 --- a/spec/fixtures/backport_pull_request.merged.json +++ b/spec/fixtures/backport_pull_request.merged.json @@ -18,7 +18,7 @@ "default_branch": "main" } }, - "body": "Backport of #14\nSee that PR for details.\nNotes: ", + "body": "Backport of #14\nSee that PR for details.\nNotes: new cool stuff added", "created_at": "2018-11-01T17:29:51Z", "merged_at": "2018-11-01T17:30:11Z", "labels": [ diff --git a/spec/fixtures/pull_request.opened.json b/spec/fixtures/pull_request.opened.json index 5269d3d..7edca67 100644 --- a/spec/fixtures/pull_request.opened.json +++ b/spec/fixtures/pull_request.opened.json @@ -10,7 +10,7 @@ "user": { "login": "codebytere" }, - "body": "New cool stuff", + "body": "New cool stuff \nNotes: new cool stuff added", "labels": [], "head": { "sha": "ABC" diff --git a/spec/index.spec.ts b/spec/index.spec.ts index 259ca9b..54c152f 100644 --- a/spec/index.spec.ts +++ b/spec/index.spec.ts @@ -430,7 +430,7 @@ Notes: `, const pr = { body: `Backport of #14 See that PR for details. -Notes: `, +Notes: new cool stuff added`, created_at: '2018-11-01T17:29:51Z', head: { ref: '123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg', diff --git a/spec/utils.spec.ts b/spec/utils.spec.ts index 2fb7b03..e9f765b 100644 --- a/spec/utils.spec.ts +++ b/spec/utils.spec.ts @@ -1,8 +1,15 @@ -import * as logUtils from '../src/utils/log-util'; import { LogLevel } from '../src/enums'; -import { tagBackportReviewers } from '../src/utils'; +import { + tagBackportReviewers, + isValidManualBackportReleaseNotes, +} from '../src/utils'; +import * as utils from '../src/utils'; +import * as logUtils from '../src/utils/log-util'; const backportPROpenedEvent = require('./fixtures/backport_pull_request.opened.json'); +const backportPRMergedEvent = require('./fixtures/backport_pull_request.merged.json'); +const PROpenedEvent = require('./fixtures/pull_request.opened.json'); +const PRClosedEvent = require('./fixtures/pull_request.closed.json'); jest.mock('../src/constants', () => ({ ...jest.requireActual('../src/constants'), @@ -74,4 +81,51 @@ describe('utils', () => { ); }); }); + + describe('isValidManualBackportReleaseNotes', () => { + const backportPRMissingReleaseNotes = backportPROpenedEvent; + const backportPRWithReleaseNotes = backportPRMergedEvent; + const originalPRWithReleaseNotes = PROpenedEvent.payload.pull_request; + const originalPRMissingReleaseNotes = PRClosedEvent.payload.pull_request; + const originalPRWithReleaseNotes2 = + backportPRMergedEvent.payload.pull_request; + + it('should return valid if release notes match original PR for single PR', async () => { + const context = { ...backportPRWithReleaseNotes }; + expect( + await isValidManualBackportReleaseNotes(context, [ + originalPRWithReleaseNotes, + ]), + ).toBe(true); + }); + + it('should return valid if release notes match original PR for multiple PR', async () => { + const context = { ...backportPRWithReleaseNotes }; + expect( + await isValidManualBackportReleaseNotes(context, [ + originalPRWithReleaseNotes, + originalPRMissingReleaseNotes, + ]), + ).toBe(true); + }); + + it('should return not valid if release notes do not match original PR for single PR', async () => { + const context = { ...backportPRMissingReleaseNotes }; + expect( + await isValidManualBackportReleaseNotes(context, [ + originalPRWithReleaseNotes, + ]), + ).toBe(false); + }); + + it('should return not valid if release notes do not match original PR for multiple PR', async () => { + const context = { ...backportPRMissingReleaseNotes }; + expect( + await isValidManualBackportReleaseNotes(context, [ + originalPRWithReleaseNotes, + originalPRWithReleaseNotes2, + ]), + ).toBe(false); + }); + }); }); diff --git a/src/index.ts b/src/index.ts index fb3f796..e25d6ab 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,6 +4,7 @@ import { backportImpl, getPRNumbersFromPRBody, isAuthorizedUser, + isValidManualBackportReleaseNotes, labelClosedPR, } from './utils'; import { @@ -249,8 +250,6 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { ]; const FASTTRACK_LABELS: string[] = ['fast-track 🚅']; - const failureMap = new Map(); - // There are several types of PRs which might not target main yet which are // inherently valid; e.g roller-bot PRs. Check for and allow those here. if (oldPRNumbers.length === 0) { @@ -286,7 +285,9 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { robot.log( `#${pr.number} has backport numbers - checking their validity now`, ); + const failureMap = new Map(); const supported = await getSupportedBranches(context); + const oldPRs = []; for (const oldPRNumber of oldPRNumbers) { robot.log(`Checking validity of #${oldPRNumber}`); @@ -296,6 +297,8 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { }), ); + oldPRs.push(oldPR); + // The current PR is only valid if the PR it is backporting // was merged to main or to a supported release branch. if ( @@ -312,29 +315,48 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { failureMap.set(oldPRNumber, cause); } } - } - for (const oldPRNumber of oldPRNumbers) { - if (failureMap.has(oldPRNumber)) { - robot.log( - `#${pr.number} is targeting a branch that is not ${ - pr.base.repo.default_branch - } - ${failureMap.get(oldPRNumber)}`, + for (const oldPRNumber of oldPRNumbers) { + if (failureMap.has(oldPRNumber)) { + robot.log( + `#${pr.number} is targeting a branch that is not ${ + pr.base.repo.default_branch + } - ${failureMap.get(oldPRNumber)}`, + ); + await updateBackportValidityCheck(context, checkRun, { + title: 'Invalid Backport', + summary: `This PR is targeting a branch that is not ${ + pr.base.repo.default_branch + } but ${failureMap.get(oldPRNumber)}`, + conclusion: CheckRunStatus.FAILURE, + }); + } else { + robot.log(`#${pr.number} is a valid backport of #${oldPRNumber}`); + await updateBackportValidityCheck(context, checkRun, { + title: 'Valid Backport', + summary: `This PR is declared as backporting "#${oldPRNumber}" which is a valid PR that has been merged into ${pr.base.repo.default_branch}`, + conclusion: CheckRunStatus.SUCCESS, + }); + } + } + + if (['opened', 'edited'].includes(action)) { + robot.log(`Checking validity of release notes`); + // Check to make sure backport PR has the same release notes as at least one of the old prs + const isValidReleaseNotes = await isValidManualBackportReleaseNotes( + context, + oldPRs as WebHookPR[], ); - await updateBackportValidityCheck(context, checkRun, { - title: 'Invalid Backport', - summary: `This PR is targeting a branch that is not ${ - pr.base.repo.default_branch - } but ${failureMap.get(oldPRNumber)}`, - conclusion: CheckRunStatus.FAILURE, - }); - } else { - robot.log(`#${pr.number} is a valid backport of #${oldPRNumber}`); - await updateBackportValidityCheck(context, checkRun, { - title: 'Valid Backport', - summary: `This PR is declared as backporting "#${oldPRNumber}" which is a valid PR that has been merged into ${pr.base.repo.default_branch}`, - conclusion: CheckRunStatus.SUCCESS, - }); + + if (!isValidReleaseNotes) { + await updateBackportValidityCheck(context, checkRun, { + title: 'Invalid Backport', + summary: `The release notes of the backport PR do not match those of ${oldPRNumbers + .map((pr) => `#${pr}`) + .join(', ')}.`, + conclusion: CheckRunStatus.FAILURE, + }); + } } } } else { diff --git a/src/operations/update-manual-backport.ts b/src/operations/update-manual-backport.ts index a28b14b..6113a38 100644 --- a/src/operations/update-manual-backport.ts +++ b/src/operations/update-manual-backport.ts @@ -4,7 +4,7 @@ import { SKIP_CHECK_LABEL, } from '../constants'; import { PRChange, PRStatus, LogLevel } from '../enums'; -import { WebHookPRContext } from '../types'; +import { WebHookPR, WebHookPRContext } from '../types'; import { isSemverMinorPR, tagBackportReviewers } from '../utils'; import * as labelUtils from '../utils/label-utils'; import { log } from '../utils/log-util'; diff --git a/src/utils.ts b/src/utils.ts index cf205e2..5e9b745 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -27,9 +27,10 @@ import { SimpleWebHookRepoContext, WebHookIssueContext, WebHookPR, + WebHookPRContext, WebHookRepoContext, } from './types'; -import { Context, Probot } from 'probot'; +import { Probot } from 'probot'; const { parse: parseDiff } = require('what-the-diff'); @@ -298,7 +299,7 @@ export const getPRNumbersFromPRBody = (pr: WebHookPR, checkNotBot = false) => { * @param context Context * @param pr Pull Request */ -const getOriginalBackportNumber = async ( +export const getOriginalBackportNumber = async ( context: SimpleWebHookRepoContext, pr: WebHookPR, ) => { @@ -365,6 +366,23 @@ const checkUserHasWriteAccess = async ( return ['write', 'admin'].includes(userInfo.permission); }; +const getReleaseNotes = (pr: Pick) => { + const onelineMatch = pr.body?.match( + /(?:(?:\r?\n)|^)notes: (.+?)(?:(?:\r?\n)|$)/gi, + ); + const multilineMatch = pr.body?.match( + /(?:(?:\r?\n)Notes:(?:\r?\n)((?:\*.+(?:(?:\r?\n)|$))+))/gi, + ); + + if (onelineMatch && onelineMatch[0]) { + return `\n\n${onelineMatch[0]}`; + } else if (multilineMatch && multilineMatch[0]) { + return `\n\n${multilineMatch[0]}`; + } else { + return '\n\nNotes: no-notes'; + } +}; + const createBackportComment = async ( context: SimpleWebHookRepoContext, pr: WebHookPR, @@ -377,23 +395,9 @@ const createBackportComment = async ( `Creating backport comment for #${prNumber}`, ); - let body = `Backport of #${prNumber}\n\nSee that PR for details.`; - - const onelineMatch = pr.body?.match( - /(?:(?:\r?\n)|^)notes: (.+?)(?:(?:\r?\n)|$)/gi, - ); - const multilineMatch = pr.body?.match( - /(?:(?:\r?\n)Notes:(?:\r?\n)((?:\*.+(?:(?:\r?\n)|$))+))/gi, - ); - + const releaseNotes = getReleaseNotes(pr); // attach release notes to backport PR body - if (onelineMatch && onelineMatch[0]) { - body += `\n\n${onelineMatch[0]}`; - } else if (multilineMatch && multilineMatch[0]) { - body += `\n\n${multilineMatch[0]}`; - } else { - body += '\n\nNotes: no-notes'; - } + const body = `Backport of #${prNumber}\n\nSee that PR for details.${releaseNotes}`; return body; }; @@ -798,3 +802,11 @@ export const backportImpl = async ( }, ); }; + +export const isValidManualBackportReleaseNotes = async ( + context: WebHookPRContext, + oldPRs: WebHookPR[], +) => { + const backportPRReleaseNotes = getReleaseNotes(context.payload.pull_request); + return oldPRs.some((pr) => getReleaseNotes(pr) === backportPRReleaseNotes); +};