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.

Sign up for GitHub

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

Jump to bottom

fix: perform pr-update when requested regardless of pr-cache state #21148

Merged
merged 4 commits into from
Mar 25, 2023
Merged

fix: perform pr-update when requested regardless of pr-cache state #21148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a9b5b9
return correct pr-info when pr updated
RahulGautamSingh Mar 25, 2023
7953316
Merge branch 'main' into fix/rebase-checkboc-issue-pr-cache
RahulGautamSingh Mar 25, 2023
780444f
return existing pr info when dryRun
RahulGautamSingh Mar 25, 2023
25d3afd
Merge branch 'main' into fix/rebase-checkboc-issue-pr-cache
RahulGautamSingh Mar 25, 2023
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
64 changes: 51 additions & 13 deletions lib/workers/repository/update/pr/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import type { PrCache } from '../../../../util/cache/repository/types';
import { fingerprint } from '../../../../util/fingerprint';
import * as _limits from '../../../global/limits';
import type { BranchConfig, BranchUpgradeConfig } from '../../../types';
import { embedChangelog } from '../../changelog';
import { embedChangelogs } from '../../changelog';
import * as _statusChecks from '../branch/status-checks';
import * as _prBody from './body';
import type { ChangeLogChange, ChangeLogRelease } from './changelog/types';
Expand Down Expand Up @@ -270,12 +270,12 @@ describe('workers/repository/update/pr/index', () => {

describe('Update', () => {
it('updates PR due to title change', async () => {
const changedPr: Pr = { ...pr, title: 'Another title' };
const changedPr: Pr = { ...pr, title: 'Another title' }; // user changed the prTitle
platform.getBranchPr.mockResolvedValueOnce(changedPr);

const res = await ensurePr(config);

expect(res).toEqual({ type: 'with-pr', pr: changedPr });
expect(res).toEqual({ type: 'with-pr', pr }); // we redo the prTitle as per config
expect(platform.updatePr).toHaveBeenCalled();
expect(platform.createPr).not.toHaveBeenCalled();
expect(logger.logger.info).toHaveBeenCalledWith(
Expand All @@ -288,13 +288,13 @@ describe('workers/repository/update/pr/index', () => {
it('updates PR due to body change', async () => {
const changedPr: Pr = {
...pr,
bodyStruct: getPrBodyStruct(`${body} updated`),
bodyStruct: getPrBodyStruct(`${body} updated`), // user changed prBody
};
platform.getBranchPr.mockResolvedValueOnce(changedPr);

const res = await ensurePr(config);

expect(res).toEqual({ type: 'with-pr', pr: changedPr });
expect(res).toEqual({ type: 'with-pr', pr }); // we redo the prBody as per config
expect(platform.updatePr).toHaveBeenCalled();
expect(platform.createPr).not.toHaveBeenCalled();
expect(prCache.setPrCache).toHaveBeenCalled();
Expand Down Expand Up @@ -803,10 +803,7 @@ describe('workers/repository/update/pr/index', () => {
});

it('updates pr-cache when pr fingerprint is different', async () => {
platform.getBranchPr.mockResolvedValue({
...existingPr,
title: 'Another title',
});
platform.getBranchPr.mockResolvedValue(existingPr);
cachedPr = {
fingerprint: 'old',
lastEdited: new Date('2020-01-20T00:00:00Z').toISOString(),
Expand All @@ -815,7 +812,7 @@ describe('workers/repository/update/pr/index', () => {
const res = await ensurePr(config);
expect(res).toEqual({
type: 'with-pr',
pr: { ...existingPr, title: 'Another title' },
pr: existingPr,
});
expect(logger.logger.debug).toHaveBeenCalledWith(
'PR fingerprints mismatch, processing PR'
Expand All @@ -827,19 +824,60 @@ describe('workers/repository/update/pr/index', () => {
config.repositoryCache = 'enabled';
platform.getBranchPr.mockResolvedValue(existingPr);
cachedPr = {
fingerprint: fingerprint(generatePrFingerprintConfig(config)),
fingerprint: fingerprint(
generatePrFingerprintConfig({ ...config, fetchReleaseNotes: true })
),
lastEdited: new Date('2020-01-20T00:00:00Z').toISOString(),
};
prCache.getPrCache.mockReturnValueOnce(cachedPr);
const res = await ensurePr(config);
const res = await ensurePr({ ...config, fetchReleaseNotes: true });
expect(res).toEqual({
type: 'with-pr',
pr: existingPr,
});
expect(logger.logger.debug).toHaveBeenCalledWith(
'PR cache matches and no PR changes in last 24hrs, so skipping PR body check'
);
expect(embedChangelog).toHaveBeenCalledTimes(0);
expect(embedChangelogs).toHaveBeenCalledTimes(0);
});

it('updates PR when rebase requested by user regardless of pr-cache state', async () => {
config.repositoryCache = 'enabled';
platform.getBranchPr.mockResolvedValue({
number,
sourceBranch,
title: prTitle,
bodyStruct: {
hash: 'hash-with-checkbox-checked',
rebaseRequested: true,
},
state: 'open',
});
cachedPr = {
fingerprint: fingerprint(
generatePrFingerprintConfig({ ...config, fetchReleaseNotes: true })
),
lastEdited: new Date('2020-01-20T00:00:00Z').toISOString(),
};
prCache.getPrCache.mockReturnValueOnce(cachedPr);
const res = await ensurePr({ ...config, fetchReleaseNotes: true });
expect(res).toEqual({
type: 'with-pr',
pr: {
number,
sourceBranch,
title: prTitle,
bodyStruct,
state: 'open',
},
});
expect(logger.logger.debug).toHaveBeenCalledWith(
'PR rebase requested, so skipping cache check'
);
expect(logger.logger.debug).not.toHaveBeenCalledWith(
`Pull Request #${number} does not need updating`
);
expect(embedChangelogs).toHaveBeenCalledTimes(1);
});

it('logs when cache is enabled but pr-cache is absent', async () => {
Expand Down
19 changes: 16 additions & 3 deletions lib/workers/repository/update/pr/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ import {
platform,
} from '../../../../modules/platform';
import { ensureComment } from '../../../../modules/platform/comment';
import { hashBody } from '../../../../modules/platform/pr-body';
import {
getPrBodyStruct,
hashBody,
} from '../../../../modules/platform/pr-body';
import { scm } from '../../../../modules/platform/scm';
import { ExternalHostError } from '../../../../types/errors/external-host-error';
import { getElapsedHours } from '../../../../util/date';
Expand Down Expand Up @@ -119,7 +122,9 @@ export async function ensurePr(
const prCache = getPrCache(branchName);
if (existingPr) {
logger.debug('Found existing PR');
if (prCache) {
if (existingPr.bodyStruct?.rebaseRequested) {
logger.debug('PR rebase requested, so skipping cache check');
} else if (prCache) {
logger.trace({ prCache }, 'Found existing PR cache');
// return if pr cache is valid and pr was not changed in the past 24hrs
if (validatePrCache(prCache, prFingerprint)) {
Expand Down Expand Up @@ -354,6 +359,7 @@ export async function ensurePr(
}
if (GlobalConfig.get('dryRun')) {
logger.info(`DRY-RUN: Would update PR #${existingPr.number}`);
return { type: 'with-pr', pr: existingPr };
} else {
await platform.updatePr({
number: existingPr.number,
Expand All @@ -364,7 +370,14 @@ export async function ensurePr(
logger.info({ pr: existingPr.number, prTitle }, `PR updated`);
setPrCache(branchName, prFingerprint, true);
}
return { type: 'with-pr', pr: existingPr };
return {
type: 'with-pr',
pr: {
...existingPr,
bodyStruct: getPrBodyStruct(prBody),
title: prTitle,
rarkins marked this conversation as resolved.
Show resolved Hide resolved
},
};
}
logger.debug({ branch: branchName, prTitle }, `Creating PR`);
if (config.updateType === 'rollback') {
Expand Down