Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: idempotency issues when rangeStrategy=update-lockfile is used with other rangeStrategy in the same file #11328

Merged
merged 15 commits into from Sep 7, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 59 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,62 @@ 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 false if does not need rebasing but has postUpdateOptions', async () => {
onigoetz marked this conversation as resolved.
Show resolved Hide resolved
config.postUpdateOptions = ['yarnDedupeFewer'];
git.branchExists.mockReturnValueOnce(true);
platform.getBranchPr.mockResolvedValueOnce({
...pr,
isConflicted: false,
});
const res = await shouldReuseExistingBranch(config);
expect(res.reuseExistingBranch).toBe(false);
});
onigoetz marked this conversation as resolved.
Show resolved Hide resolved

it('returns true if unmergeable and cannot rebase', async () => {
git.branchExists.mockReturnValueOnce(true);
platform.getBranchPr.mockResolvedValueOnce({
Expand Down
44 changes: 42 additions & 2 deletions lib/workers/branch/reuse.ts
@@ -1,16 +1,16 @@
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';
onigoetz marked this conversation as resolved.
Show resolved Hide resolved

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 +86,45 @@ 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 = {};
onigoetz marked this conversation as resolved.
Show resolved Hide resolved
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) {
viceice marked this conversation as resolved.
Show resolved Hide resolved
logger.debug(`Branch is using postUpdateOptions`);
return {
reuseExistingBranch: false,
isModified: false,
};
}

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