From 9087662e5f80d0a0033fc2c9d3c505ce3411b1f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Goetz?= Date: Wed, 25 Aug 2021 23:14:07 +0200 Subject: [PATCH] fix: disable branch reuse when other rangeStrategies are used along with update-lockfile --- lib/workers/branch/reuse.spec.ts | 49 ++++++++++++++++++++++++++++++-- lib/workers/branch/reuse.ts | 43 ++++++++++++++++++++++------ 2 files changed, 82 insertions(+), 10 deletions(-) diff --git a/lib/workers/branch/reuse.spec.ts b/lib/workers/branch/reuse.spec.ts index 9c3edf14b3ead6..e45a2bdbb8401d 100644 --- a/lib/workers/branch/reuse.spec.ts +++ b/lib/workers/branch/reuse.spec.ts @@ -1,7 +1,7 @@ import { git, platform } from '../../../test/util'; -import type { RenovateConfig } from '../../config/types'; import { Pr } from '../../platform'; import { PrState } from '../../types'; +import type { BranchConfig } from '../types'; import { shouldReuseExistingBranch } from './reuse'; jest.mock('../../util/git'); @@ -13,12 +13,13 @@ describe('workers/branch/reuse', () => { state: PrState.Open, title: 'any', }; - let config: RenovateConfig; + let config: BranchConfig; beforeEach(() => { config = { branchName: 'renovate/some-branch', rebaseLabel: 'rebase', rebaseWhen: 'behind-base-branch', + upgrades: [], }; jest.resetAllMocks(); }); @@ -43,6 +44,50 @@ describe('workers/branch/reuse', () => { expect(res.reuseExistingBranch).toBe(true); }); + it('returns false if does not need rebasing but has upgrades that need lockfile maintenance along with upgrades that do not', async () => { + config.upgrades = [ + { + packageFile: 'package.json', + rangeStrategy: 'replace', + branchName: 'current', + }, + { + packageFile: 'package.json', + rangeStrategy: 'update-lockfile', + branchName: 'current', + }, + ]; + git.branchExists.mockReturnValueOnce(true); + platform.getBranchPr.mockResolvedValueOnce({ + ...pr, + isConflicted: false, + }); + const res = await shouldReuseExistingBranch(config); + expect(res.reuseExistingBranch).toBe(false); + }); + + it('returns true if does not need rebasing and lockfile update is on different packages', async () => { + config.upgrades = [ + { + packageFile: 'package.json', + rangeStrategy: 'replace', + branchName: 'current', + }, + { + packageFile: 'subpackage/package.json', + rangeStrategy: 'update-lockfile', + branchName: 'current', + }, + ]; + git.branchExists.mockReturnValueOnce(true); + platform.getBranchPr.mockResolvedValueOnce({ + ...pr, + isConflicted: false, + }); + const res = await shouldReuseExistingBranch(config); + expect(res.reuseExistingBranch).toBe(true); + }); + it('returns false if does not need rebasing but has postUpdateOptions', async () => { config.postUpdateOptions = ['yarnDedupeFewer']; git.branchExists.mockReturnValueOnce(true); diff --git a/lib/workers/branch/reuse.ts b/lib/workers/branch/reuse.ts index 2a91696e76401f..654403f71a7478 100644 --- a/lib/workers/branch/reuse.ts +++ b/lib/workers/branch/reuse.ts @@ -1,8 +1,8 @@ import { getGlobalConfig } from '../../config/global'; -import type { RenovateConfig } from '../../config/types'; import { logger } from '../../logger'; import { platform } from '../../platform'; import { branchExists, isBranchModified, isBranchStale } from '../../util/git'; +import type { BranchConfig } from '../types'; type ParentBranch = { reuseExistingBranch: boolean; @@ -10,7 +10,7 @@ type ParentBranch = { }; export async function shouldReuseExistingBranch( - config: RenovateConfig + config: BranchConfig ): Promise { const { branchName } = config; // Check if branch exists @@ -87,12 +87,39 @@ export async function shouldReuseExistingBranch( } logger.debug(`Branch does not need rebasing`); - // When postUpdateOptions are defined, some lockfiles might be changed after processing packages - // This shouldn't be a problem but if the PR is already created and the package.json didn't change - // the second pass will only commit the files changed by postUpdateOptions (#10050) - // To avoid this we don't reuse branches is postUpdateOptions are present - // We are saved from the double push by a check at commit time that won't push identical contents - if (config.postUpdateOptions?.length > 0) { + // Branches can get in an inconsistent state if "update-lockfile" is used at the same time as other strategies + // On the first execution, everything is executed, but if on a second execution the package.json modification is + // skipped but the lockfile update is executed, the lockfile will have a different result than if it was executed + // along with the changes to the package.json. Thus ending up with an incomplete branch update + // This is why we are skipping branch reuse in this case (#10050) + const groupedByPackageFile = {}; + for (const upgrade of config.upgrades) { + groupedByPackageFile[upgrade.packageFile] = + groupedByPackageFile[upgrade.packageFile] || new Set(); + groupedByPackageFile[upgrade.packageFile].add(upgrade.rangeStrategy); + + if ( + groupedByPackageFile[upgrade.packageFile].size > 1 && + groupedByPackageFile[upgrade.packageFile].has('update-lockfile') + ) { + logger.debug( + `Detected multiple rangeStrategies along with update-lockfile` + ); + return { + reuseExistingBranch: false, + isModified: false, + }; + } + } + + // Branches can get in an inconsistent state if postUpdateOptions is used. + // package.json updates are run conditionally, but postUpdateOptions are run everytime + // On the first execution, everything is executed, but if on a second execution the package.json modification is + // skipped but the postUpdateOptions is executed, the lockfile will have a different result than if it was executed + // along with the changes to the package.json. Thus ending up with an incomplete branch update + // This is why we are skipping branch reuse when postUpdateOptions is used (#10050) + if ((config.postUpdateOptions as string[])?.length > 0) { + logger.debug(`Branch is using postUpdateOptions`); return { reuseExistingBranch: false, isModified: false,