Skip to content

Commit

Permalink
fix: idempotency issues when rangeStrategy=update-lockfile is used wi…
Browse files Browse the repository at this point in the history
…th other rangeStrategy in the same file (#11328)
  • Loading branch information
onigoetz committed Sep 7, 2021
1 parent 8785f70 commit 2a60a9c
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 4 deletions.
50 changes: 48 additions & 2 deletions 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');
Expand All @@ -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();
});
Expand All @@ -42,6 +43,51 @@ describe('workers/branch/reuse', () => {
const res = await shouldReuseExistingBranch(config);
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 true if unmergeable and cannot rebase', async () => {
git.branchExists.mockReturnValueOnce(true);
platform.getBranchPr.mockResolvedValueOnce({
Expand Down
31 changes: 29 additions & 2 deletions lib/workers/branch/reuse.ts
@@ -1,16 +1,17 @@
import { getGlobalConfig } from '../../config/global';
import type { RenovateConfig } from '../../config/types';
import { logger } from '../../logger';
import { platform } from '../../platform';
import type { RangeStrategy } from '../../types';
import { branchExists, isBranchModified, isBranchStale } from '../../util/git';
import type { BranchConfig } from '../types';

type ParentBranch = {
reuseExistingBranch: boolean;
isModified?: boolean;
};

export async function shouldReuseExistingBranch(
config: RenovateConfig
config: BranchConfig
): Promise<ParentBranch> {
const { branchName } = config;
// Check if branch exists
Expand Down Expand Up @@ -86,5 +87,31 @@ export async function shouldReuseExistingBranch(
logger.debug(`Branch is not mergeable but can't be rebased`);
}
logger.debug(`Branch does not need rebasing`);

// 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: Record<string, Set<RangeStrategy>> = {};
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,
};
}
}

return { reuseExistingBranch: true, isModified: false };
}

0 comments on commit 2a60a9c

Please sign in to comment.