From 5d028bad7caa7d583c0944abe1c0f711affbbb54 Mon Sep 17 00:00:00 2001 From: Alice Zhao Date: Sun, 31 Mar 2024 22:40:22 -0700 Subject: [PATCH 1/5] fix: add default reviewers to manual backport --- src/operations/update-manual-backport.ts | 9 ++++- src/utils.ts | 51 +++++++++++++++--------- 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/src/operations/update-manual-backport.ts b/src/operations/update-manual-backport.ts index 261cacd..8f81760 100644 --- a/src/operations/update-manual-backport.ts +++ b/src/operations/update-manual-backport.ts @@ -6,7 +6,7 @@ import { BACKPORT_REQUESTED_LABEL, SKIP_CHECK_LABEL, } from '../constants'; -import { isSemverMinorPR } from '../utils'; +import { tagBackportReviewers, isSemverMinorPR } from '../utils'; import { WebHookPRContext } from '../types'; /** @@ -135,6 +135,13 @@ please check out #${pr.number}`; }), ); } + + // Tag default reviewers to manual backport + await tagBackportReviewers({ + context, + user: pr.user.login, + targetPrNumber: pr.number, + }); } else if (type === PRChange.MERGE) { log( 'updateManualBackport', diff --git a/src/utils.ts b/src/utils.ts index 955f454..cd23a0c 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -397,6 +397,34 @@ const createBackportComment = async ( return body; }; +export const tagBackportReviewers = async ({ + context, + user, + targetPrNumber, +}: { + context: SimpleWebHookRepoContext; + user: string; + targetPrNumber: number; +}) => { + const reviewers = []; + const hasWrite = await checkUserHasWriteAccess(context, user); + // Optionally request a default review team for backports. + // If the PR author has write access, also request their review. + if (hasWrite) reviewers.push(user); + if (DEFAULT_BACKPORT_REVIEW_TEAM) { + reviewers.push(DEFAULT_BACKPORT_REVIEW_TEAM); + } + + if (reviewers.length > 0) { + await context.octokit.pulls.requestReviewers( + context.repo({ + pull_number: targetPrNumber, + reviewers, + }), + ); + } +}; + export const backportImpl = async ( robot: Probot, context: SimpleWebHookRepoContext, @@ -561,24 +589,11 @@ export const backportImpl = async ( }), ); - const reviewers = []; - const hasWrite = await checkUserHasWriteAccess(context, pr.user.login); - - // Optionally request a default review team for backports. - // If the PR author has write access, also request their review. - if (hasWrite) reviewers.push(pr.user.login); - if (DEFAULT_BACKPORT_REVIEW_TEAM) { - reviewers.push(DEFAULT_BACKPORT_REVIEW_TEAM); - } - - if (reviewers.length > 0) { - await context.octokit.pulls.requestReviewers( - context.repo({ - pull_number: newPr.number, - reviewers, - }), - ); - } + await tagBackportReviewers({ + context, + user: pr.user.login, + targetPrNumber: newPr.number, + }); log( 'backportImpl', From fdf22845cdf1eaac19848cd2fbf7927544c8125d Mon Sep 17 00:00:00 2001 From: Alice Zhao Date: Sun, 31 Mar 2024 22:40:51 -0700 Subject: [PATCH 2/5] test: add test to updateManualBackport --- .../backport_pull_request.closed.json | 6 ++ .../backport_pull_request.merged.json | 6 ++ spec/index.spec.ts | 12 ++++ spec/operations.spec.ts | 61 +++++++++++++++++++ 4 files changed, 85 insertions(+) diff --git a/spec/fixtures/backport_pull_request.closed.json b/spec/fixtures/backport_pull_request.closed.json index fc5ff68..c910da9 100644 --- a/spec/fixtures/backport_pull_request.closed.json +++ b/spec/fixtures/backport_pull_request.closed.json @@ -12,6 +12,12 @@ "head": { "ref": "123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg" }, + "base": { + "ref": "main", + "repo": { + "default_branch": "main" + } + }, "body": "Backport of #14\nSee that PR for details.\nNotes: ", "created_at": "2018-11-01T17:29:51Z", "merged_at": "2018-11-01T17:30:11Z", diff --git a/spec/fixtures/backport_pull_request.merged.json b/spec/fixtures/backport_pull_request.merged.json index e3fbe42..00f857d 100644 --- a/spec/fixtures/backport_pull_request.merged.json +++ b/spec/fixtures/backport_pull_request.merged.json @@ -12,6 +12,12 @@ "head": { "ref": "123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg" }, + "base": { + "ref": "main", + "repo": { + "default_branch": "main" + } + }, "body": "Backport of #14\nSee that PR for details.\nNotes: ", "created_at": "2018-11-01T17:29:51Z", "merged_at": "2018-11-01T17:30:11Z", diff --git a/spec/index.spec.ts b/spec/index.spec.ts index 1d856b5..259ca9b 100644 --- a/spec/index.spec.ts +++ b/spec/index.spec.ts @@ -392,6 +392,12 @@ Notes: `, head: { ref: '123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg', }, + base: { + ref: 'main', + repo: { + default_branch: 'main', + }, + }, labels: [ { color: 'ededed', @@ -429,6 +435,12 @@ Notes: `, head: { ref: '123456789iuytdxcvbnjhfdriuyfedfgy54escghjnbg', }, + base: { + ref: 'main', + repo: { + default_branch: 'main', + }, + }, labels: [ { color: 'ededed', diff --git a/spec/operations.spec.ts b/spec/operations.spec.ts index 0be51f4..23423f3 100644 --- a/spec/operations.spec.ts +++ b/spec/operations.spec.ts @@ -5,6 +5,9 @@ import * as path from 'path'; import simpleGit from 'simple-git'; import { initRepo } from '../src/operations/init-repo'; import { setupRemotes } from '../src/operations/setup-remotes'; +import { tagBackportReviewers } from '../src/utils'; +import { PRChange } from '../src/enums'; +import { updateManualBackport } from '../src/operations/update-manual-backport'; let dirObject: { dir?: string } | null = null; @@ -13,6 +16,22 @@ const saveDir = (o: { dir: string }) => { return o.dir; }; +const backportPRMergedEvent = require('./fixtures/backport_pull_request.merged.json'); +const backportPRClosedEvent = require('./fixtures/backport_pull_request.closed.json'); +const backportPROpenedEvent = require('./fixtures/backport_pull_request.opened.json'); + +jest.mock('../src/utils', () => ({ + tagBackportReviewers: jest.fn().mockReturnValue(Promise.resolve()), + isSemverMinorPR: jest.fn().mockReturnValue(false), +})); + +jest.mock('../src/utils/label-utils', () => ({ + labelExistsOnPR: jest.fn().mockResolvedValue(true), + getSemverLabel: jest.fn().mockResolvedValue(false), + addLabels: jest.fn(), + removeLabel: jest.fn(), +})); + describe('runner', () => { jest.setTimeout(30000); console.error = jest.fn(); @@ -101,4 +120,46 @@ describe('runner', () => { } }); }); + + describe('updateManualBackport()', () => { + let context: any; + let octokit = { + pulls: { + get: jest.fn().mockReturnValue(Promise.resolve({})), + }, + issues: { + createComment: jest.fn().mockReturnValue(Promise.resolve({})), + listComments: jest.fn().mockReturnValue(Promise.resolve({ data: [] })), + }, + }; + + it('tags reviewers on manual backport creation', async () => { + context = { + ...backportPROpenedEvent, + octokit, + repo: jest.fn(), + }; + await updateManualBackport(context, PRChange.OPEN, 1234); + expect(tagBackportReviewers).toHaveBeenCalled(); + }); + + it('does not tag reviewers on merged PRs', async () => { + context = { + ...backportPRMergedEvent, + octokit, + repo: jest.fn(), + }; + await updateManualBackport(context, PRChange.MERGE, 1234); + expect(tagBackportReviewers).not.toHaveBeenCalled(); + }); + it('does not tag reviewers on closed PRs', async () => { + context = { + ...backportPRClosedEvent, + octokit, + repo: jest.fn(), + }; + await updateManualBackport(context, PRChange.CLOSE, 1234); + expect(tagBackportReviewers).not.toHaveBeenCalled(); + }); + }); }); From 20271fa06573f89ccfabde3a757d4818b16d2f94 Mon Sep 17 00:00:00 2001 From: Alice Zhao Date: Mon, 1 Apr 2024 12:11:47 -0700 Subject: [PATCH 3/5] fix: pass undefined user for manual backport --- .../fixtures/backport_pull_request.opened.json | 1 + spec/operations.spec.ts | 14 ++++++++++---- spec/queue.spec.ts | 2 -- src/operations/update-manual-backport.ts | 9 ++++----- src/utils.ts | 18 +++++++++++------- 5 files changed, 26 insertions(+), 18 deletions(-) diff --git a/spec/fixtures/backport_pull_request.opened.json b/spec/fixtures/backport_pull_request.opened.json index 430ded5..75d0821 100644 --- a/spec/fixtures/backport_pull_request.opened.json +++ b/spec/fixtures/backport_pull_request.opened.json @@ -4,6 +4,7 @@ "action": "opened", "number": 7, "pull_request": { + "number": 7, "url": "my_cool_url", "title": "CHANGE README", "merged": true, diff --git a/spec/operations.spec.ts b/spec/operations.spec.ts index 23423f3..237e604 100644 --- a/spec/operations.spec.ts +++ b/spec/operations.spec.ts @@ -3,11 +3,12 @@ import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; import simpleGit from 'simple-git'; + +import { PRChange } from '../src/enums'; import { initRepo } from '../src/operations/init-repo'; import { setupRemotes } from '../src/operations/setup-remotes'; -import { tagBackportReviewers } from '../src/utils'; -import { PRChange } from '../src/enums'; import { updateManualBackport } from '../src/operations/update-manual-backport'; +import { tagBackportReviewers } from '../src/utils'; let dirObject: { dir?: string } | null = null; @@ -121,9 +122,9 @@ describe('runner', () => { }); }); - describe('updateManualBackport()', () => { + describe.only('updateManualBackport()', () => { let context: any; - let octokit = { + const octokit = { pulls: { get: jest.fn().mockReturnValue(Promise.resolve({})), }, @@ -141,6 +142,10 @@ describe('runner', () => { }; await updateManualBackport(context, PRChange.OPEN, 1234); expect(tagBackportReviewers).toHaveBeenCalled(); + expect(tagBackportReviewers).toHaveBeenCalledWith({ + context, + targetPrNumber: 7, + }); }); it('does not tag reviewers on merged PRs', async () => { @@ -152,6 +157,7 @@ describe('runner', () => { await updateManualBackport(context, PRChange.MERGE, 1234); expect(tagBackportReviewers).not.toHaveBeenCalled(); }); + it('does not tag reviewers on closed PRs', async () => { context = { ...backportPRClosedEvent, diff --git a/spec/queue.spec.ts b/spec/queue.spec.ts index 5129cee..14566f1 100644 --- a/spec/queue.spec.ts +++ b/spec/queue.spec.ts @@ -3,8 +3,6 @@ import * as sinon from 'sinon'; import { ExecutionQueue } from '../src/Queue'; -const noop = async () => {}; - const waitForEvent = (emitter: EventEmitter, event: string) => { return new Promise((resolve) => { emitter.once(event, resolve); diff --git a/src/operations/update-manual-backport.ts b/src/operations/update-manual-backport.ts index 8f81760..3111af3 100644 --- a/src/operations/update-manual-backport.ts +++ b/src/operations/update-manual-backport.ts @@ -1,13 +1,13 @@ -import * as labelUtils from '../utils/label-utils'; -import { log } from '../utils/log-util'; -import { PRChange, PRStatus, LogLevel } from '../enums'; import { BACKPORT_LABEL, BACKPORT_REQUESTED_LABEL, SKIP_CHECK_LABEL, } from '../constants'; -import { tagBackportReviewers, isSemverMinorPR } from '../utils'; +import { PRChange, PRStatus, LogLevel } from '../enums'; import { WebHookPRContext } from '../types'; +import * as labelUtils from '../utils/label-utils'; +import { isSemverMinorPR, tagBackportReviewers } from '../utils'; +import { log } from '../utils/log-util'; /** * Updates the labels on a backport's original PR as well as comments with links @@ -139,7 +139,6 @@ please check out #${pr.number}`; // Tag default reviewers to manual backport await tagBackportReviewers({ context, - user: pr.user.login, targetPrNumber: pr.number, }); } else if (type === PRChange.MERGE) { diff --git a/src/utils.ts b/src/utils.ts index cd23a0c..747af28 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -399,22 +399,26 @@ const createBackportComment = async ( export const tagBackportReviewers = async ({ context, - user, targetPrNumber, + user, }: { context: SimpleWebHookRepoContext; - user: string; targetPrNumber: number; + user?: string; }) => { const reviewers = []; - const hasWrite = await checkUserHasWriteAccess(context, user); - // Optionally request a default review team for backports. - // If the PR author has write access, also request their review. - if (hasWrite) reviewers.push(user); + if (DEFAULT_BACKPORT_REVIEW_TEAM) { reviewers.push(DEFAULT_BACKPORT_REVIEW_TEAM); } + if (user) { + const hasWrite = await checkUserHasWriteAccess(context, user); + // Optionally request a default review team for backports. + // If the PR author has write access, also request their review. + if (hasWrite) reviewers.push(user); + } + if (reviewers.length > 0) { await context.octokit.pulls.requestReviewers( context.repo({ @@ -591,8 +595,8 @@ export const backportImpl = async ( await tagBackportReviewers({ context, - user: pr.user.login, targetPrNumber: newPr.number, + user: pr.user.login, }); log( From 39aeaacb29acafe9acfb458d11315cb560b8d3b8 Mon Sep 17 00:00:00 2001 From: Alice Zhao Date: Mon, 1 Apr 2024 13:10:37 -0700 Subject: [PATCH 4/5] chore: remove .only and clean up imports --- spec/operations.spec.ts | 4 ++-- src/operations/update-manual-backport.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/operations.spec.ts b/spec/operations.spec.ts index 237e604..9a23532 100644 --- a/spec/operations.spec.ts +++ b/spec/operations.spec.ts @@ -17,8 +17,8 @@ const saveDir = (o: { dir: string }) => { return o.dir; }; -const backportPRMergedEvent = require('./fixtures/backport_pull_request.merged.json'); const backportPRClosedEvent = require('./fixtures/backport_pull_request.closed.json'); +const backportPRMergedEvent = require('./fixtures/backport_pull_request.merged.json'); const backportPROpenedEvent = require('./fixtures/backport_pull_request.opened.json'); jest.mock('../src/utils', () => ({ @@ -122,7 +122,7 @@ describe('runner', () => { }); }); - describe.only('updateManualBackport()', () => { + describe('updateManualBackport()', () => { let context: any; const octokit = { pulls: { diff --git a/src/operations/update-manual-backport.ts b/src/operations/update-manual-backport.ts index 3111af3..d2b1b67 100644 --- a/src/operations/update-manual-backport.ts +++ b/src/operations/update-manual-backport.ts @@ -5,8 +5,8 @@ import { } from '../constants'; import { PRChange, PRStatus, LogLevel } from '../enums'; import { WebHookPRContext } from '../types'; -import * as labelUtils from '../utils/label-utils'; import { isSemverMinorPR, tagBackportReviewers } from '../utils'; +import * as labelUtils from '../utils/label-utils'; import { log } from '../utils/log-util'; /** From 46f574259f32e8c412517e5672ad705a4a70563c Mon Sep 17 00:00:00 2001 From: Alice Zhao Date: Mon, 1 Apr 2024 13:26:33 -0700 Subject: [PATCH 5/5] test: use const for context variable in updateManualbackport test --- spec/operations.spec.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/operations.spec.ts b/spec/operations.spec.ts index 9a23532..1f4c971 100644 --- a/spec/operations.spec.ts +++ b/spec/operations.spec.ts @@ -123,7 +123,6 @@ describe('runner', () => { }); describe('updateManualBackport()', () => { - let context: any; const octokit = { pulls: { get: jest.fn().mockReturnValue(Promise.resolve({})), @@ -135,7 +134,7 @@ describe('runner', () => { }; it('tags reviewers on manual backport creation', async () => { - context = { + const context = { ...backportPROpenedEvent, octokit, repo: jest.fn(), @@ -149,7 +148,7 @@ describe('runner', () => { }); it('does not tag reviewers on merged PRs', async () => { - context = { + const context = { ...backportPRMergedEvent, octokit, repo: jest.fn(), @@ -159,7 +158,7 @@ describe('runner', () => { }); it('does not tag reviewers on closed PRs', async () => { - context = { + const context = { ...backportPRClosedEvent, octokit, repo: jest.fn(),