diff --git a/spec/fixtures/backport_pull_request.opened.json b/spec/fixtures/backport_pull_request.opened.json index 75d0821..af596b9 100644 --- a/spec/fixtures/backport_pull_request.opened.json +++ b/spec/fixtures/backport_pull_request.opened.json @@ -7,7 +7,7 @@ "number": 7, "url": "my_cool_url", "title": "CHANGE README", - "merged": true, + "merged": false, "user": { "login": "codebytere" }, diff --git a/spec/operations.spec.ts b/spec/operations.spec.ts index 1f4c971..5bb7c51 100644 --- a/spec/operations.spec.ts +++ b/spec/operations.spec.ts @@ -22,8 +22,9 @@ const backportPRMergedEvent = require('./fixtures/backport_pull_request.merged.j const backportPROpenedEvent = require('./fixtures/backport_pull_request.opened.json'); jest.mock('../src/utils', () => ({ - tagBackportReviewers: jest.fn().mockReturnValue(Promise.resolve()), + tagBackportReviewers: jest.fn(), isSemverMinorPR: jest.fn().mockReturnValue(false), + needsSemverMinorBackportLabel: jest.fn().mockReturnValue(false), })); jest.mock('../src/utils/label-utils', () => ({ diff --git a/spec/utils.spec.ts b/spec/utils.spec.ts index 2fb7b03..7094385 100644 --- a/spec/utils.spec.ts +++ b/spec/utils.spec.ts @@ -1,8 +1,14 @@ -import * as logUtils from '../src/utils/log-util'; import { LogLevel } from '../src/enums'; -import { tagBackportReviewers } from '../src/utils'; +import { + needsSemverMinorBackportLabel, + tagBackportReviewers, +} from '../src/utils'; +import * as utils from '../src/utils'; +import * as labelUtils from '../src/utils/label-utils'; +import * as logUtils from '../src/utils/log-util'; const backportPROpenedEvent = require('./fixtures/backport_pull_request.opened.json'); +const backportPRClosedEvent = require('./fixtures/backport_pull_request.closed.json'); jest.mock('../src/constants', () => ({ ...jest.requireActual('../src/constants'), @@ -74,4 +80,43 @@ describe('utils', () => { ); }); }); + + describe('needsSemverMinorBackportLabel()', () => { + const context = { + octokit: {}, + repo: {}, + ...backportPROpenedEvent, + }; + const pr = context.payload.pull_request; + + it('should should return true if PR is semver minor and not already approved', async () => { + jest.spyOn(labelUtils, 'labelExistsOnPR').mockResolvedValue(false); + jest.spyOn(utils, 'isSemverMinorPR').mockResolvedValue(true); + + expect(await needsSemverMinorBackportLabel(context, pr)).toBe(true); + }); + + it('should return false if PR is not semver minor', async () => { + jest.spyOn(utils, 'isSemverMinorPR').mockResolvedValue(false); + + expect(await needsSemverMinorBackportLabel(context, pr)).toBe(false); + }); + + it('should return false if PR is already approved', async () => { + jest.spyOn(utils, 'isSemverMinorPR').mockResolvedValue(true); + + // Mocking labelExistsOnPR to return true (indicating backport is already approved) + jest.spyOn(labelUtils, 'labelExistsOnPR').mockResolvedValue(true); + + expect(await needsSemverMinorBackportLabel(context, pr)).toBe(false); + }); + + it('should return false if PR is merged', async () => { + const closedPr = backportPRClosedEvent.payload.pull_request; + + expect(await needsSemverMinorBackportLabel(context, closedPr)).toBe( + false, + ); + }); + }); }); diff --git a/src/constants.ts b/src/constants.ts index bfc0e6f..d9ad40f 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -19,11 +19,11 @@ export const SEMVER_LABELS = { MAJOR: 'semver/major', }; -export const SKIP_CHECK_LABEL = - process.env.SKIP_CHECK_LABEL || 'backport-check-skip'; - -export const BACKPORT_REQUESTED_LABEL = - process.env.BACKPORT_REQUESTED_LABEL || 'backport/requested 🗳'; +export const BACKPORT_REVIEW_LABELS = { + SKIP: process.env.SKIP_CHECK_LABEL || 'backport-check-skip', + REQUESTED: process.env.BACKPORT_REQUESTED_LABEL || 'backport/requested 🗳', + APPROVED: 'backport/approved ✅', +}; export const DEFAULT_BACKPORT_REVIEW_TEAM = process.env.DEFAULT_BACKPORT_REVIEW_TEAM; diff --git a/src/index.ts b/src/index.ts index 1bbfd93..2c4073c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -11,7 +11,11 @@ import { labelExistsOnPR, removeLabel, } from './utils/label-utils'; -import { CHECK_PREFIX, NO_BACKPORT_LABEL, SKIP_CHECK_LABEL } from './constants'; +import { + BACKPORT_REVIEW_LABELS, + CHECK_PREFIX, + NO_BACKPORT_LABEL, +} from './constants'; import { getEnvVar } from './utils/env-util'; import { PRChange, PRStatus, BackportPurpose, CheckRunStatus } from './enums'; import { Label } from '@octokit/webhooks-types'; @@ -251,9 +255,11 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { // If a branch is targeting something that isn't main it might not be a backport; // allow for a label to skip backport validity check for these branches. - if (await labelExistsOnPR(context, pr.number, SKIP_CHECK_LABEL)) { + if ( + await labelExistsOnPR(context, pr.number, BACKPORT_REVIEW_LABELS.SKIP) + ) { robot.log( - `#${pr.number} is labeled with ${SKIP_CHECK_LABEL} - skipping backport validation check`, + `#${pr.number} is labeled with ${BACKPORT_REVIEW_LABELS.SKIP} - skipping backport validation check`, ); await updateBackportValidityCheck(context, checkRun, { title: 'Backport Check Skipped', diff --git a/src/operations/update-manual-backport.ts b/src/operations/update-manual-backport.ts index d2b1b67..e7f1d77 100644 --- a/src/operations/update-manual-backport.ts +++ b/src/operations/update-manual-backport.ts @@ -1,11 +1,7 @@ -import { - BACKPORT_LABEL, - BACKPORT_REQUESTED_LABEL, - SKIP_CHECK_LABEL, -} from '../constants'; +import { BACKPORT_LABEL, BACKPORT_REVIEW_LABELS } from '../constants'; import { PRChange, PRStatus, LogLevel } from '../enums'; import { WebHookPRContext } from '../types'; -import { isSemverMinorPR, tagBackportReviewers } from '../utils'; +import { needsSemverMinorBackportLabel, tagBackportReviewers } from '../utils'; import * as labelUtils from '../utils/label-utils'; import { log } from '../utils/log-util'; @@ -59,7 +55,7 @@ export const updateManualBackport = async ( const skipCheckLabelExists = await labelUtils.labelExistsOnPR( context, pr.number, - SKIP_CHECK_LABEL, + BACKPORT_REVIEW_LABELS.SKIP, ); if (!skipCheckLabelExists) { newPRLabelsToAdd.push(BACKPORT_LABEL); @@ -98,13 +94,14 @@ export const updateManualBackport = async ( } } - if (await isSemverMinorPR(context, pr)) { + if (await needsSemverMinorBackportLabel(context, pr)) { log( 'updateManualBackport', LogLevel.INFO, - `Determined that ${pr.number} is semver-minor`, + `Determined that ${pr.number} is semver-minor and needs backport review`, ); - newPRLabelsToAdd.push(BACKPORT_REQUESTED_LABEL); + + newPRLabelsToAdd.push(BACKPORT_REVIEW_LABELS.REQUESTED); } // We should only comment if there is not a previous existing comment diff --git a/src/utils.ts b/src/utils.ts index 7ec29cd..5042cce 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -5,9 +5,9 @@ import simpleGit from 'simple-git'; import queue from './Queue'; import { - BACKPORT_REQUESTED_LABEL, - DEFAULT_BACKPORT_REVIEW_TEAM, BACKPORT_LABEL, + BACKPORT_REVIEW_LABELS, + DEFAULT_BACKPORT_REVIEW_TEAM, } from './constants'; import { PRStatus, BackportPurpose, LogLevel, PRChange } from './enums'; @@ -640,13 +640,14 @@ export const backportImpl = async ( const labelsToAdd = [BACKPORT_LABEL, `${targetBranch}`]; - if (await isSemverMinorPR(context, pr)) { + if (await needsSemverMinorBackportLabel(context, pr)) { log( 'backportImpl', LogLevel.INFO, - `Determined that ${pr.number} is semver-minor`, + `Determined that ${pr.number} is semver-minor and needs backport review`, ); - labelsToAdd.push(BACKPORT_REQUESTED_LABEL); + + labelsToAdd.push(BACKPORT_REVIEW_LABELS.REQUESTED); } const semverLabel = labelUtils.getSemverLabel(pr); @@ -787,3 +788,25 @@ export const backportImpl = async ( }, ); }; + +export const needsSemverMinorBackportLabel = async ( + context: SimpleWebHookRepoContext, + pr: WebHookPR, +) => { + if (pr.merged) { + return false; + } + + if (!(await isSemverMinorPR(context, pr))) { + return false; + } + + // Check to see if backport has already been approved. + const hasApprovedLabel = await labelUtils.labelExistsOnPR( + context, + pr.number, + BACKPORT_REVIEW_LABELS.APPROVED, + ); + + return !hasApprovedLabel; +};