feat: add validation check for manual bp release notes

spec/fixtures/backport_pull_request.merged.json

@@ -18,7 +18,7 @@
           "default_branch": "main"
         }
       },
-      "body": "Backport of #14\nSee that PR for details.\nNotes: <!-- One-line Change Summary Here-->",
+      "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": [

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"

spec/index.spec.ts

@@ -430,7 +430,7 @@ Notes: <!-- One-line Change Summary Here-->`,
       const pr = {
         body: `Backport of #14
 See that PR for details.
-Notes: <!-- One-line Change Summary Here-->`,
+Notes: new cool stuff added`,
         created_at: '2018-11-01T17:29:51Z',
         head: {
           ref: '123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg',

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);
+    });
+  });
 });

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 {

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';

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<WebHookPR, 'body'>) => {
+  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);
+};