From 62c68d07e31624d838fb4692c18fca802b3587b6 Mon Sep 17 00:00:00 2001 From: Sergei Zharinov <zharinov@users.noreply.github.com> Date: Wed, 16 Dec 2020 12:13:52 +0400 Subject: [PATCH] refactor(workers): Use limiting API for PRs instead of in-place counters (#8031) --- lib/workers/branch/index.spec.ts | 15 ++++++--- lib/workers/branch/index.ts | 11 +++---- lib/workers/global/limits.ts | 1 + lib/workers/pr/index.spec.ts | 18 ++++++----- lib/workers/pr/index.ts | 6 ++-- lib/workers/repository/process/write.spec.ts | 32 +++++++------------- lib/workers/repository/process/write.ts | 30 +++++++----------- 7 files changed, 52 insertions(+), 61 deletions(-) diff --git a/lib/workers/branch/index.spec.ts b/lib/workers/branch/index.spec.ts index fd6d88eb20..1c755c898c 100644 --- a/lib/workers/branch/index.spec.ts +++ b/lib/workers/branch/index.spec.ts @@ -9,6 +9,7 @@ import { PrState } from '../../types'; import * as _exec from '../../util/exec'; import { File, StatusResult } from '../../util/git'; import { BranchConfig, PrResult, ProcessBranchResult } from '../common'; +import * as _limits from '../global/limits'; import * as _prWorker from '../pr'; import * as _automerge from './automerge'; import * as _checkExisting from './check-existing'; @@ -29,6 +30,7 @@ jest.mock('../pr'); jest.mock('../../util/exec'); jest.mock('../../util/git'); jest.mock('fs-extra'); +jest.mock('../global/limits'); const getUpdated = mocked(_getUpdated); const schedule = mocked(_schedule); @@ -40,6 +42,7 @@ const commit = mocked(_commit); const prWorker = mocked(_prWorker); const exec = mocked(_exec); const fs = mocked(_fs); +const limits = mocked(_limits); describe('workers/branch', () => { describe('processBranch', () => { @@ -215,8 +218,9 @@ describe('workers/branch', () => { artifactErrors: [], updatedArtifacts: [], }); - git.branchExists.mockReturnValue(false); - expect(await branchWorker.processBranch(config, true)).toEqual( + limits.isLimitReached.mockReturnValueOnce(true); + limits.isLimitReached.mockReturnValueOnce(false); + expect(await branchWorker.processBranch(config)).toEqual( ProcessBranchResult.PrLimitReached ); }); @@ -232,7 +236,8 @@ describe('workers/branch', () => { prWorker.ensurePr.mockResolvedValueOnce({ prResult: PrResult.LimitReached, }); - expect(await branchWorker.processBranch(config, true)).toEqual( + limits.isLimitReached.mockReturnValue(false); + expect(await branchWorker.processBranch(config)).toEqual( ProcessBranchResult.PrLimitReached ); }); @@ -245,7 +250,9 @@ describe('workers/branch', () => { updatedArtifacts: [], }); git.branchExists.mockReturnValue(false); - expect(await branchWorker.processBranch(config, false, true)).toEqual( + limits.isLimitReached.mockReturnValueOnce(false); + limits.isLimitReached.mockReturnValueOnce(true); + expect(await branchWorker.processBranch(config)).toEqual( ProcessBranchResult.CommitLimitReached ); }); diff --git a/lib/workers/branch/index.ts b/lib/workers/branch/index.ts index 8650a2d585..e5ce7680eb 100644 --- a/lib/workers/branch/index.ts +++ b/lib/workers/branch/index.ts @@ -30,6 +30,7 @@ import { import { regEx } from '../../util/regex'; import * as template from '../../util/template'; import { BranchConfig, PrResult, ProcessBranchResult } from '../common'; +import { Limit, isLimitReached } from '../global/limits'; import { checkAutoMerge, ensurePr, getPlatformPrOptions } from '../pr'; import { tryBranchAutomerge } from './automerge'; import { prAlreadyExisted } from './check-existing'; @@ -60,9 +61,7 @@ async function deleteBranchSilently(branchName: string): Promise<void> { } export async function processBranch( - branchConfig: BranchConfig, - prLimitReached?: boolean, - commitLimitReached?: boolean + branchConfig: BranchConfig ): Promise<ProcessBranchResult> { const config: BranchConfig = { ...branchConfig }; const dependencies = config.upgrades @@ -153,7 +152,7 @@ export async function processBranch( } if ( !branchExists && - prLimitReached && + isLimitReached(Limit.PullRequests) && !dependencyDashboardCheck && !config.vulnerabilityAlert ) { @@ -161,7 +160,7 @@ export async function processBranch( return ProcessBranchResult.PrLimitReached; } if ( - commitLimitReached && + isLimitReached(Limit.Commits) && !dependencyDashboardCheck && !config.vulnerabilityAlert ) { @@ -583,7 +582,7 @@ export async function processBranch( logger.debug( `There are ${config.errors.length} errors and ${config.warnings.length} warnings` ); - const { prResult: result, pr } = await ensurePr(config, prLimitReached); + const { prResult: result, pr } = await ensurePr(config); if (result === PrResult.LimitReached) { logger.debug('Reached PR limit - skipping PR creation'); return ProcessBranchResult.PrLimitReached; diff --git a/lib/workers/global/limits.ts b/lib/workers/global/limits.ts index 04d194c72d..c5476f8922 100644 --- a/lib/workers/global/limits.ts +++ b/lib/workers/global/limits.ts @@ -2,6 +2,7 @@ import { logger } from '../../logger'; export enum Limit { Commits = 'Commits', + PullRequests = 'PullRequests', } interface LimitValue { diff --git a/lib/workers/pr/index.spec.ts b/lib/workers/pr/index.spec.ts index 59ee12f457..cd5ec032bf 100644 --- a/lib/workers/pr/index.spec.ts +++ b/lib/workers/pr/index.spec.ts @@ -4,6 +4,7 @@ import { PLATFORM_TYPE_GITLAB } from '../../constants/platforms'; import { Pr, platform as _platform } from '../../platform'; import { BranchStatus } from '../../types'; import { BranchConfig, PrResult } from '../common'; +import * as _limits from '../global/limits'; import * as _changelogHelper from './changelog'; import { getChangeLogJSON } from './changelog'; import * as codeOwners from './code-owners'; @@ -14,10 +15,12 @@ const changelogHelper = mocked(_changelogHelper); const gitlabChangelogHelper = mocked(_changelogHelper); const platform = mocked(_platform); const defaultConfig = getConfig(); +const limits = mocked(_limits); jest.mock('../../util/git'); jest.mock('./changelog'); jest.mock('./code-owners'); +jest.mock('../global/limits'); function setupChangelogMock() { changelogHelper.getChangeLogJSON = jest.fn(); @@ -268,7 +271,8 @@ describe('workers/pr', () => { config.prCreation = 'status-success'; config.automerge = true; config.schedule = ['before 5am']; - const { prResult } = await prWorker.ensurePr(config, true); + limits.isLimitReached.mockReturnValueOnce(true); + const { prResult } = await prWorker.ensurePr(config); expect(prResult).toEqual(PrResult.LimitReached); expect(platform.createPr.mock.calls).toBeEmpty(); }); @@ -278,13 +282,11 @@ describe('workers/pr', () => { config.prCreation = 'status-success'; config.automerge = true; config.schedule = ['before 5am']; - const { prResult } = await prWorker.ensurePr( - { - ...config, - dependencyDashboardChecks: { 'renovate/dummy-1.x': 'true' }, - }, - true - ); + limits.isLimitReached.mockReturnValueOnce(true); + const { prResult } = await prWorker.ensurePr({ + ...config, + dependencyDashboardChecks: { 'renovate/dummy-1.x': 'true' }, + }); expect(prResult).toEqual(PrResult.Created); expect(platform.createPr).toHaveBeenCalled(); }); diff --git a/lib/workers/pr/index.ts b/lib/workers/pr/index.ts index 068874f390..7d991b3367 100644 --- a/lib/workers/pr/index.ts +++ b/lib/workers/pr/index.ts @@ -15,6 +15,7 @@ import { isBranchModified, } from '../../util/git'; import { BranchConfig, PrResult } from '../common'; +import { Limit, isLimitReached } from '../global/limits'; import { getPrBody } from './body'; import { ChangeLogError } from './changelog'; import { codeOwnersForPr } from './code-owners'; @@ -114,8 +115,7 @@ export function getPlatformPrOptions( // Ensures that PR exists with matching title/body export async function ensurePr( - prConfig: BranchConfig, - prLimitReached?: boolean + prConfig: BranchConfig ): Promise<{ prResult: PrResult; pr?: Pr; @@ -381,7 +381,7 @@ export async function ensurePr( logger.info('DRY-RUN: Would create PR: ' + prTitle); pr = { number: 0, displayNumber: 'Dry run PR' } as never; } else { - if (!dependencyDashboardCheck && prLimitReached) { + if (!dependencyDashboardCheck && isLimitReached(Limit.PullRequests)) { return { prResult: PrResult.LimitReached }; } pr = await platform.createPr({ diff --git a/lib/workers/repository/process/write.spec.ts b/lib/workers/repository/process/write.spec.ts index db7620c79b..378544c3bb 100644 --- a/lib/workers/repository/process/write.spec.ts +++ b/lib/workers/repository/process/write.spec.ts @@ -1,6 +1,7 @@ import { RenovateConfig, getConfig, git, mocked } from '../../../../test/util'; import * as _branchWorker from '../../branch'; import { BranchConfig, ProcessBranchResult } from '../../common'; +import { Limit, isLimitReached } from '../../global/limits'; import * as _limits from './limits'; import { writeUpdates } from './write'; @@ -57,28 +58,17 @@ describe('workers/repository/write', () => { expect(res).toEqual('automerged'); expect(branchWorker.processBranch).toHaveBeenCalledTimes(4); }); - it('counts created branches', async () => { - const branches: BranchConfig[] = [ - { res: ProcessBranchResult.Pending, expect: false }, - { res: ProcessBranchResult.PrLimitReached, expect: true }, - ] as never; - limits.getPrsRemaining.mockResolvedValueOnce(1); - - branches.forEach(({ res, expect: limitReached }) => { - branchWorker.processBranch.mockResolvedValueOnce(res); - git.branchExists.mockReturnValueOnce(false); - git.branchExists.mockReturnValueOnce(!limitReached as never); - }); - - await writeUpdates(config, branches); - - branches.forEach((branch) => - expect(branchWorker.processBranch).toHaveBeenCalledWith( - branch, - branch.expect, - false - ) + it('increments branch counter', async () => { + const branches: BranchConfig[] = [{}] as never; + branchWorker.processBranch.mockResolvedValueOnce( + ProcessBranchResult.Pending ); + git.branchExists.mockReturnValueOnce(false); + git.branchExists.mockReturnValueOnce(true); + limits.getPrsRemaining.mockResolvedValueOnce(1); + expect(isLimitReached(Limit.PullRequests)).toBeFalse(); + await writeUpdates({ config }, branches); + expect(isLimitReached(Limit.PullRequests)).toBeTrue(); }); }); }); diff --git a/lib/workers/repository/process/write.ts b/lib/workers/repository/process/write.ts index 74a86a7686..19c2bb98a7 100644 --- a/lib/workers/repository/process/write.ts +++ b/lib/workers/repository/process/write.ts @@ -5,7 +5,7 @@ import { PrState } from '../../../types'; import { branchExists } from '../../../util/git'; import { processBranch } from '../../branch'; import { BranchConfig, ProcessBranchResult } from '../../common'; -import { Limit, isLimitReached } from '../../global/limits'; +import { Limit, incLimitedValue, setMaxLimit } from '../../global/limits'; import { getPrsRemaining } from './limits'; export type WriteUpdateResult = 'done' | 'automerged'; @@ -39,15 +39,14 @@ export async function writeUpdates( } return true; }); - let prsRemaining = await getPrsRemaining(config, branches); + const prsRemaining = await getPrsRemaining(config, branches); logger.debug({ prsRemaining }, 'Calculated maximum PRs remaining this run'); + setMaxLimit(Limit.PullRequests, prsRemaining); for (const branch of branches) { addMeta({ branch: branch.branchName }); - const prLimitReached = prsRemaining <= 0; - const commitLimitReached = isLimitReached(Limit.Commits); const branchExisted = branchExists(branch.branchName); const prExisted = await prExists(branch.branchName); - const res = await processBranch(branch, prLimitReached, commitLimitReached); + const res = await processBranch(branch); branch.res = res; if ( res === ProcessBranchResult.Automerged && @@ -56,34 +55,27 @@ export async function writeUpdates( // Stop processing other branches because base branch has been changed return 'automerged'; } - let deductPrRemainingCount = 0; if (res === ProcessBranchResult.PrCreated) { - deductPrRemainingCount = 1; + incLimitedValue(Limit.PullRequests); } // istanbul ignore if - if ( + else if ( res === ProcessBranchResult.Automerged && branch.automergeType === 'pr-comment' && branch.requiredStatusChecks === null ) { - deductPrRemainingCount = 1; - } - if ( + incLimitedValue(Limit.PullRequests); + } else if ( res === ProcessBranchResult.Pending && !branchExisted && branchExists(branch.branchName) ) { - deductPrRemainingCount = 1; + incLimitedValue(Limit.PullRequests); } // istanbul ignore if - if ( - deductPrRemainingCount === 0 && - !prExisted && - (await prExists(branch.branchName)) - ) { - deductPrRemainingCount = 1; + else if (!prExisted && (await prExists(branch.branchName))) { + incLimitedValue(Limit.PullRequests); } - prsRemaining -= deductPrRemainingCount; } removeMeta(['branch']); return 'done'; -- GitLab