From f5db234e9b794d30b5e4e097a8c7391d4aa8cde0 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Wed, 21 Aug 2024 20:16:09 +0530 Subject: [PATCH 01/31] init --- .../config-migration/branch/index.ts | 73 +++++++++++-------- .../repository/config-migration/index.ts | 44 ++++++----- .../repository/config-migration/pr/index.ts | 16 ++-- .../repository/dependency-dashboard.ts | 2 + lib/workers/repository/index.ts | 9 ++- 5 files changed, 88 insertions(+), 56 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index 507fada5d197a9..aa8b8d3ffded0e 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -3,7 +3,6 @@ import type { RenovateConfig } from '../../../../config/types'; import { logger } from '../../../../logger'; import type { FindPRConfig, Pr } from '../../../../modules/platform'; import { platform } from '../../../../modules/platform'; -import { ensureComment } from '../../../../modules/platform/comment'; import { scm } from '../../../../modules/platform/scm'; import { getMigrationBranchName } from '../common'; import { ConfigMigrationCommitMessageFactory } from './commit-message'; @@ -11,15 +10,32 @@ import { createConfigMigrationBranch } from './create'; import type { MigratedData } from './migrated-data'; import { rebaseMigrationBranch } from './rebase'; +interface CheckConfigMigrationBranchResult { + migrationBranch?: string; + result?: string; + prNumber?: number; +} + export async function checkConfigMigrationBranch( config: RenovateConfig, migratedConfigData: MigratedData | null, -): Promise { +): Promise { logger.debug('checkConfigMigrationBranch()'); if (!migratedConfigData) { logger.debug('checkConfigMigrationBranch() Config does not need migration'); - return null; + return { result: 'nothing' }; } + + if ( + !config.configMigration && + !config.dependencyDashboardChecks?.createMigrationPr + ) { + logger.debug( + 'Config migration needed but config migration is disabled and checkbox not ticked.', + ); + return { result: 'add-checkbox' }; + } + const configMigrationBranch = getMigrationBranchName(config); const branchPr = await migrationPrExists( @@ -45,12 +61,23 @@ export async function checkConfigMigrationBranch( // found closed migration PR if (closedPr) { + logger.debug('Closed config migration PR found.'); + + // previously we used to add a comment and skip the branch + // but now we add a checkbox in DD + // if checkbox is ticked then remove this branch/pr and create a new one + + if (!config.dependencyDashboardChecks?.createMigrationPr) { + logger.debug( + 'Closed PR and config migration enabled. Adding checkbox to DD, will check in next run.', + ); + return { result: 'add-checkbox' }; + } + logger.debug( - { prTitle: closedPr.title }, - 'Closed PR already exists. Skipping branch.', + 'Closed migration PR found and checkbox is ticked so delete this branch and create a new one.', ); await handlePr(config, closedPr); - return null; } } @@ -74,40 +101,26 @@ export async function checkConfigMigrationBranch( if (!GlobalConfig.get('dryRun')) { await scm.checkoutBranch(configMigrationBranch); } - return configMigrationBranch; + return { + migrationBranch: configMigrationBranch, + result: 'pr-created', + prNumber: branchPr?.number, + }; } export async function migrationPrExists( branchName: string, targetBranch?: string, -): Promise { - return !!(await platform.getBranchPr(branchName, targetBranch)); +): Promise { + return await platform.getBranchPr(branchName, targetBranch); } async function handlePr(config: RenovateConfig, pr: Pr): Promise { - if ( - pr.state === 'closed' && - !config.suppressNotifications!.includes('prIgnoreNotification') - ) { + if (await scm.branchExists(pr.sourceBranch)) { if (GlobalConfig.get('dryRun')) { - logger.info( - `DRY-RUN: Would ensure closed PR comment in PR #${pr.number}`, - ); + logger.info('DRY-RUN: Would delete branch ' + pr.sourceBranch); } else { - const content = - '\n\nIf you accidentally closed this PR, or if you changed your mind: rename this PR to get a fresh replacement PR.'; - await ensureComment({ - number: pr.number, - topic: 'Renovate Ignore Notification', - content, - }); - } - if (await scm.branchExists(pr.sourceBranch)) { - if (GlobalConfig.get('dryRun')) { - logger.info('DRY-RUN: Would delete branch ' + pr.sourceBranch); - } else { - await scm.deleteBranch(pr.sourceBranch); - } + await scm.deleteBranch(pr.sourceBranch); } } } diff --git a/lib/workers/repository/config-migration/index.ts b/lib/workers/repository/config-migration/index.ts index 856b0ceb277268..ff3f3d9210b370 100644 --- a/lib/workers/repository/config-migration/index.ts +++ b/lib/workers/repository/config-migration/index.ts @@ -1,29 +1,37 @@ import type { RenovateConfig } from '../../../config/types'; import { logger } from '../../../logger'; +import type { Pr } from '../../../modules/platform'; import { checkConfigMigrationBranch } from './branch'; import { MigratedDataFactory } from './branch/migrated-data'; import { ensureConfigMigrationPr } from './pr'; +export interface ConfigMigrationResult { + migrationBranch?: string; + result?: string; + prNumber?: number; +} + export async function configMigration( config: RenovateConfig, branchList: string[], -): Promise { - if (config.configMigration) { - if (config.mode === 'silent') { - logger.debug( - 'Config migration issues are not created, updated or closed when mode=silent', - ); - return; - } - const migratedConfigData = await MigratedDataFactory.getAsync(); - const migrationBranch = await checkConfigMigrationBranch( - config, - migratedConfigData, - ); // null if migration not needed - if (migrationBranch) { - branchList.push(migrationBranch); - await ensureConfigMigrationPr(config, migratedConfigData!); - } - MigratedDataFactory.reset(); +): Promise { + if (config.mode === 'silent') { + logger.debug( + 'Config migration issues are not created, updated or closed when mode=silent', + ); + return {}; + } + + let pr: Pr | null = null; + const migratedConfigData = await MigratedDataFactory.getAsync(); + const { migrationBranch, result, prNumber } = + await checkConfigMigrationBranch(config, migratedConfigData); // null if migration not needed + + if (migrationBranch) { + branchList.push(migrationBranch); + pr = await ensureConfigMigrationPr(config, migratedConfigData!); } + MigratedDataFactory.reset(); + + return { migrationBranch, result, prNumber: pr?.number ?? prNumber }; } diff --git a/lib/workers/repository/config-migration/pr/index.ts b/lib/workers/repository/config-migration/pr/index.ts index 53a3ccb57744cd..e8fb39f0a319c4 100644 --- a/lib/workers/repository/config-migration/pr/index.ts +++ b/lib/workers/repository/config-migration/pr/index.ts @@ -2,7 +2,7 @@ import is from '@sindresorhus/is'; import { GlobalConfig } from '../../../../config/global'; import type { RenovateConfig } from '../../../../config/types'; import { logger } from '../../../../logger'; -import { platform } from '../../../../modules/platform'; +import { Pr, platform } from '../../../../modules/platform'; import { hashBody } from '../../../../modules/platform/pr-body'; import { scm } from '../../../../modules/platform/scm'; import { emojify } from '../../../../util/emoji'; @@ -19,7 +19,7 @@ import { getMigrationBranchName } from '../common'; export async function ensureConfigMigrationPr( config: RenovateConfig, migratedConfigData: MigratedData, -): Promise { +): Promise { logger.debug('ensureConfigMigrationPr()'); const docsLink = joinUrlParts( coerceString(config.productLinks?.documentation), @@ -74,7 +74,7 @@ ${ existingPr.title === prTitle ) { logger.debug(`Pr does not need updating, PrNo: ${existingPr.number}`); - return; + return existingPr; } // PR must need updating if (GlobalConfig.get('dryRun')) { @@ -87,7 +87,7 @@ ${ }); logger.info({ pr: existingPr.number }, 'Migration PR updated'); } - return; + return existingPr; } logger.debug('Creating migration PR'); const labels = prepareLabels(config); @@ -111,6 +111,8 @@ ${ if (pr) { await addParticipants(config, pr); } + + return pr; } } catch (err) { if ( @@ -124,8 +126,10 @@ ${ 'Migration PR already exists but cannot find it. It was probably created by a different user.', ); await scm.deleteBranch(branchName); - return; + return null; } - throw err; + return null; } + + return null; } diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index d23e5e70beb298..00ec86ec8a8fd5 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -9,6 +9,7 @@ import { coerceString } from '../../util/string'; import * as template from '../../util/template'; import type { BranchConfig, SelectAllConfig } from '../types'; import { extractRepoProblems } from './common'; +import type { ConfigMigrationResult } from './config-migration'; import { getDepWarningsDashboard } from './errors-warnings'; import { PackageFiles } from './package-files'; import type { Vulnerability } from './process/types'; @@ -178,6 +179,7 @@ export async function ensureDependencyDashboard( config: SelectAllConfig, allBranches: BranchConfig[], packageFiles: Record = {}, + configMigrationRes: ConfigMigrationResult, ): Promise { logger.debug('ensureDependencyDashboard()'); if (config.mode === 'silent') { diff --git a/lib/workers/repository/index.ts b/lib/workers/repository/index.ts index 25536aad048604..81d244e7fa931b 100644 --- a/lib/workers/repository/index.ts +++ b/lib/workers/repository/index.ts @@ -102,8 +102,13 @@ export async function renovateRepository( } logger.debug(`Automerged but already retried once`); } else { - await configMigration(config, branchList); - await ensureDependencyDashboard(config, branches, packageFiles); + const configMigrationRes = await configMigration(config, branchList); + await ensureDependencyDashboard( + config, + branches, + packageFiles, + configMigrationRes, + ); } await finalizeRepo(config, branchList); // TODO #22198 From 6cadf70034002938deb196586fbc60e7afe4d015 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Fri, 23 Aug 2024 06:01:29 +0530 Subject: [PATCH 02/31] add checkbox code --- .../repository/config-migration/branch/index.ts | 15 +++++++++------ lib/workers/repository/config-migration/index.ts | 2 +- lib/workers/repository/dependency-dashboard.ts | 13 +++++++++++++ 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index aa8b8d3ffded0e..c7ed1c5ae1a27a 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -12,7 +12,7 @@ import { rebaseMigrationBranch } from './rebase'; interface CheckConfigMigrationBranchResult { migrationBranch?: string; - result?: string; + result?: 'add-checkbox' | 'pr-exists'; prNumber?: number; } @@ -23,12 +23,12 @@ export async function checkConfigMigrationBranch( logger.debug('checkConfigMigrationBranch()'); if (!migratedConfigData) { logger.debug('checkConfigMigrationBranch() Config does not need migration'); - return { result: 'nothing' }; + return {}; } if ( !config.configMigration && - !config.dependencyDashboardChecks?.createMigrationPr + !config.dependencyDashboardChecks?.createConfigMigrationPr ) { logger.debug( 'Config migration needed but config migration is disabled and checkbox not ticked.', @@ -65,9 +65,12 @@ export async function checkConfigMigrationBranch( // previously we used to add a comment and skip the branch // but now we add a checkbox in DD - // if checkbox is ticked then remove this branch/pr and create a new one + // if checkbox is ticked then remove this branch/pr and create new one - if (!config.dependencyDashboardChecks?.createMigrationPr) { + if ( + !config.dependencyDashboardChecks?.createConfigMigrationPr || + config.dependencyDashboardChecks?.createConfigMigrationPr === 'no' + ) { logger.debug( 'Closed PR and config migration enabled. Adding checkbox to DD, will check in next run.', ); @@ -103,7 +106,7 @@ export async function checkConfigMigrationBranch( } return { migrationBranch: configMigrationBranch, - result: 'pr-created', + result: 'pr-exists', prNumber: branchPr?.number, }; } diff --git a/lib/workers/repository/config-migration/index.ts b/lib/workers/repository/config-migration/index.ts index ff3f3d9210b370..6b493fef33a2b3 100644 --- a/lib/workers/repository/config-migration/index.ts +++ b/lib/workers/repository/config-migration/index.ts @@ -7,7 +7,7 @@ import { ensureConfigMigrationPr } from './pr'; export interface ConfigMigrationResult { migrationBranch?: string; - result?: string; + result?: 'add-checkbox' | 'pr-exists'; prNumber?: number; } diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 00ec86ec8a8fd5..3ba27ce8194ad7 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -48,6 +48,10 @@ function checkRebaseAll(issueBody: string): boolean { return issueBody.includes(' - [x] '); } +function checkCreateConfigMigrationPr(issueBody: string): boolean { + return issueBody.includes(' - [x] '); +} + function selectAllRelevantBranches(issueBody: string): string[] { const checkedBranches = []; if (checkOpenAllRateLimitedPR(issueBody)) { @@ -93,6 +97,8 @@ function parseDashboardIssue(issueBody: string): DependencyDashboard { const dependencyDashboardAllPending = checkApproveAllPendingPR(issueBody); const dependencyDashboardAllRateLimited = checkOpenAllRateLimitedPR(issueBody); + dependencyDashboardChecks['createConfigMigrationPr'] = + checkCreateConfigMigrationPr(issueBody) ? 'yes' : 'no'; return { dependencyDashboardChecks, dependencyDashboardRebaseAllOpen, @@ -265,6 +271,13 @@ export async function ensureDependencyDashboard( return; } let issueBody = ''; + + if (configMigrationRes.result === 'pr-exists') { + issueBody += `Config Migration PR Number: #${configMigrationRes.prNumber}\n\n`; + } else if (configMigrationRes.result === 'add-checkbox') { + issueBody += + ' - [ ] Create Migration PR' + '\n\n'; + } if (config.dependencyDashboardHeader?.length) { issueBody += template.compile(config.dependencyDashboardHeader, config) + '\n\n'; From e7c182fe74341fe721c4fa190bed488611b3b17f Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Wed, 28 Aug 2024 20:05:08 +0530 Subject: [PATCH 03/31] update dashboard text --- lib/workers/repository/dependency-dashboard.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 3ba27ce8194ad7..2e8dc369f1b78a 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -273,10 +273,11 @@ export async function ensureDependencyDashboard( let issueBody = ''; if (configMigrationRes.result === 'pr-exists') { - issueBody += `Config Migration PR Number: #${configMigrationRes.prNumber}\n\n`; + issueBody += `Config Migration necessary. You can view the Config Migration PR here #${configMigrationRes.prNumber}\n\n`; } else if (configMigrationRes.result === 'add-checkbox') { issueBody += - ' - [ ] Create Migration PR' + '\n\n'; + ' - [ ] Config Migration necessary. Please tick this checkbox to create an automated Config Migration PR' + + '\n\n'; } if (config.dependencyDashboardHeader?.length) { issueBody += From aca2d946d127d4ba4ab35a477c369fdb6588d07a Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Wed, 28 Aug 2024 23:59:39 +0530 Subject: [PATCH 04/31] refactor --- lib/workers/repository/dependency-dashboard.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 2e8dc369f1b78a..7c868680b90523 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -185,7 +185,7 @@ export async function ensureDependencyDashboard( config: SelectAllConfig, allBranches: BranchConfig[], packageFiles: Record = {}, - configMigrationRes: ConfigMigrationResult, + configMigrationRes?: ConfigMigrationResult, ): Promise { logger.debug('ensureDependencyDashboard()'); if (config.mode === 'silent') { @@ -272,13 +272,14 @@ export async function ensureDependencyDashboard( } let issueBody = ''; - if (configMigrationRes.result === 'pr-exists') { + if (configMigrationRes?.result === 'pr-exists') { issueBody += `Config Migration necessary. You can view the Config Migration PR here #${configMigrationRes.prNumber}\n\n`; - } else if (configMigrationRes.result === 'add-checkbox') { + } else if (configMigrationRes?.result === 'add-checkbox') { issueBody += ' - [ ] Config Migration necessary. Please tick this checkbox to create an automated Config Migration PR' + '\n\n'; } + if (config.dependencyDashboardHeader?.length) { issueBody += template.compile(config.dependencyDashboardHeader, config) + '\n\n'; From cc9c35c70ea9512e5c3e5ff45799a86d74e9042b Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Thu, 29 Aug 2024 17:49:27 +0530 Subject: [PATCH 05/31] add tests --- .../config-migration/branch/index.spec.ts | 255 +++++++++++++----- .../config-migration/branch/index.ts | 7 +- .../repository/dependency-dashboard.ts | 20 +- 3 files changed, 201 insertions(+), 81 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.spec.ts b/lib/workers/repository/config-migration/branch/index.spec.ts index b0f9f6170c3785..98ef7534ffde5a 100644 --- a/lib/workers/repository/config-migration/branch/index.spec.ts +++ b/lib/workers/repository/config-migration/branch/index.spec.ts @@ -4,7 +4,7 @@ import type { RenovateConfig } from '../../../../../test/util'; import { git, mockedFunction, - partial, + // partial, platform, scm, } from '../../../../../test/util'; @@ -37,63 +37,143 @@ describe('workers/repository/config-migration/branch/index', () => { config.branchPrefix = 'some/'; }); - it('Exits when Migration is not needed', async () => { + // exists when migration not needed + it('exits when migration is not needed', async () => { await expect( checkConfigMigrationBranch(config, null), - ).resolves.toBeNull(); + ).resolves.toBeEmptyObject(); expect(logger.debug).toHaveBeenCalledWith( 'checkConfigMigrationBranch() Config does not need migration', ); }); - it('Updates migration branch & refresh PR', async () => { - platform.getBranchPr.mockResolvedValue(mock()); - // platform.refreshPr is undefined as it is an optional function - // declared as: refreshPr?(number: number): Promise; + // returns result:add-checkbox when migration is disabled but needed or checkbox is not ticked + it('returns add checkbox message when migration disabled and checkbox unchecked', async () => { + await expect( + checkConfigMigrationBranch( + { + ...config, + configMigration: false, + dependencyDashboardChecks: { configMigrationInfo: 'unchecked' }, + }, + migratedData, + ), + ).resolves.toMatchObject({ result: 'add-checkbox' }); + expect(logger.debug).toHaveBeenCalledWith( + 'Config migration needed but config migration is disabled and checkbox not ticked.', + ); + }); + + // return result: pr-exists, when migration branch name & pr number when migration disabled & checkbox ticked or open pr exists + it('creates migration branch when migration disabled but checkbox checked', async () => { + mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( + 'committed', + ); + await expect( + checkConfigMigrationBranch( + { + ...config, + configMigration: false, + dependencyDashboardChecks: { configMigrationInfo: 'checked' }, + }, + migratedData, + ), + ).resolves.toMatchObject({ + result: 'pr-exists', + migrationBranch: `${config.branchPrefix!}migrate-config`, + }); + expect(logger.debug).toHaveBeenCalledWith('Need to create migration PR'); + }); + + it('updates migration branch & refreshes pr when migration disabled but open pr exists', async () => { + platform.getBranchPr.mockResolvedValue( + mock({ + number: 1, + }), + ); platform.refreshPr = jest.fn().mockResolvedValueOnce(null); mockedFunction(rebaseMigrationBranch).mockResolvedValueOnce('committed'); - const res = await checkConfigMigrationBranch(config, migratedData); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: false, + dependencyDashboardChecks: { + configMigrationInfo: 'migration-pr-exists', + }, + }, + migratedData, + ); // TODO: types (#22198) - expect(res).toBe(`${config.branchPrefix!}migrate-config`); + expect(res).toMatchObject({ + result: 'pr-exists', + migrationBranch: `${config.branchPrefix!}migrate-config`, + prNumber: 1, + }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); expect(git.commitFiles).toHaveBeenCalledTimes(0); + expect(platform.refreshPr).toHaveBeenCalledTimes(1); expect(logger.debug).toHaveBeenCalledWith( 'Config Migration PR already exists', ); }); - it('Dry runs update migration branch', async () => { - GlobalConfig.set({ - dryRun: 'full', - }); - platform.getBranchPr.mockResolvedValueOnce(mock()); - mockedFunction(rebaseMigrationBranch).mockResolvedValueOnce('committed'); - const res = await checkConfigMigrationBranch(config, migratedData); + // return result: pr-exists, migrationBranchName & prNum when migration enabled and no pr or open pr exists + it('creates migration branch when migration enabled but no pr exists', async () => { + mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( + 'committed', + ); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: true, + dependencyDashboardChecks: { + configMigrationInfo: 'unchecked', + }, + }, + migratedData, + ); // TODO: types (#22198) - expect(res).toBe(`${config.branchPrefix!}migrate-config`); - expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); + expect(res).toMatchObject({ + result: 'pr-exists', + migrationBranch: `${config.branchPrefix!}migrate-config`, + }); + expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); expect(git.commitFiles).toHaveBeenCalledTimes(0); + expect(logger.debug).toHaveBeenCalledWith('Need to create migration PR'); }); - it('Creates migration PR', async () => { - mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( - 'committed', + it('updates migration branch & refresh PR when migration enabled and open pr exists', async () => { + platform.getBranchPr.mockResolvedValue(mock()); + platform.refreshPr = jest.fn().mockResolvedValueOnce(null); + mockedFunction(rebaseMigrationBranch).mockResolvedValueOnce('committed'); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: true, + dependencyDashboardChecks: { + configMigrationInfo: 'unchecked', + }, + }, + migratedData, ); - const res = await checkConfigMigrationBranch(config, migratedData); // TODO: types (#22198) - expect(res).toBe(`${config.branchPrefix!}migrate-config`); + expect(res).toMatchObject({ + result: 'pr-exists', + migrationBranch: `${config.branchPrefix!}migrate-config`, + }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); expect(git.commitFiles).toHaveBeenCalledTimes(0); - expect(logger.debug).toHaveBeenCalledWith('Need to create migration PR'); + expect(logger.debug).toHaveBeenCalledWith( + 'Config Migration PR already exists', + ); }); - it('Dry runs create migration PR', async () => { + it('Dry runs update migration branch', async () => { GlobalConfig.set({ dryRun: 'full', }); - mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( - 'committed', - ); + platform.getBranchPr.mockResolvedValueOnce(mock()); + mockedFunction(rebaseMigrationBranch).mockResolvedValueOnce('committed'); const res = await checkConfigMigrationBranch(config, migratedData); // TODO: types (#22198) expect(res).toBe(`${config.branchPrefix!}migrate-config`); @@ -101,51 +181,80 @@ describe('workers/repository/config-migration/branch/index', () => { expect(git.commitFiles).toHaveBeenCalledTimes(0); }); - describe('handle closed PR', () => { - const title = 'PR title'; - const pr = partial({ title, state: 'closed', number: 1 }); - - it('skips branch when there is a closed one delete it and add an ignore PR message', async () => { - platform.findPr.mockResolvedValueOnce(pr); - platform.getBranchPr.mockResolvedValue(null); - scm.branchExists.mockResolvedValueOnce(true); - const res = await checkConfigMigrationBranch(config, migratedData); - expect(res).toBeNull(); - expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); - expect(scm.commitAndPush).toHaveBeenCalledTimes(0); - expect(scm.deleteBranch).toHaveBeenCalledTimes(1); - expect(logger.debug).toHaveBeenCalledWith( - { prTitle: title }, - 'Closed PR already exists. Skipping branch.', - ); - expect(platform.ensureComment).toHaveBeenCalledTimes(1); - expect(platform.ensureComment).toHaveBeenCalledWith({ - content: - '\n\nIf you accidentally closed this PR, or if you changed your mind: rename this PR to get a fresh replacement PR.', - topic: 'Renovate Ignore Notification', - number: 1, - }); - }); + // it('Creates migration PR', async () => { + // mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( + // 'committed', + // ); + // const res = await checkConfigMigrationBranch(config, migratedData); + // // TODO: types (#22198) + // expect(res).toBe(`${config.branchPrefix!}migrate-config`); + // expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); + // expect(git.commitFiles).toHaveBeenCalledTimes(0); + // expect(logger.debug).toHaveBeenCalledWith('Need to create migration PR'); + // }); - it('dryrun: skips branch when there is a closed one and add an ignore PR message', async () => { - GlobalConfig.set({ dryRun: 'full' }); - platform.findPr.mockResolvedValueOnce(pr); - platform.getBranchPr.mockResolvedValue(null); - scm.branchExists.mockResolvedValueOnce(true); - const res = await checkConfigMigrationBranch(config, migratedData); - expect(res).toBeNull(); - expect(logger.info).toHaveBeenCalledWith( - `DRY-RUN: Would ensure closed PR comment in PR #${pr.number}`, - ); - expect(logger.info).toHaveBeenCalledWith( - 'DRY-RUN: Would delete branch ' + pr.sourceBranch, - ); - expect(logger.debug).toHaveBeenCalledWith( - { prTitle: title }, - 'Closed PR already exists. Skipping branch.', - ); - expect(platform.ensureComment).toHaveBeenCalledTimes(0); - }); - }); + // it('Dry runs create migration PR', async () => { + // GlobalConfig.set({ + // dryRun: 'full', + // }); + // mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( + // 'committed', + // ); + // const res = await checkConfigMigrationBranch(config, migratedData); + // // TODO: types (#22198) + // expect(res).toBe(`${config.branchPrefix!}migrate-config`); + // expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); + // expect(git.commitFiles).toHaveBeenCalledTimes(0); + // }); + + // describe('handle closed PR', () => { + // const title = 'PR title'; + // const pr = partial({ title, state: 'closed', number: 1 }); + + // returns result:add-checkbox when migration disabled and closed migration pr exists and checkbox not ticked + // return result: add-checkbox, when migration enabled but closed pr exists + + // it('skips branch when there is a closed one delete it and add an ignore PR message', async () => { + // platform.findPr.mockResolvedValueOnce(pr); + // platform.getBranchPr.mockResolvedValue(null); + // scm.branchExists.mockResolvedValueOnce(true); + // const res = await checkConfigMigrationBranch(config, migratedData); + // expect(res).toBeNull(); + // expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); + // expect(scm.commitAndPush).toHaveBeenCalledTimes(0); + // expect(scm.deleteBranch).toHaveBeenCalledTimes(1); + // expect(logger.debug).toHaveBeenCalledWith( + // { prTitle: title }, + // 'Closed PR already exists. Skipping branch.', + // ); + // expect(platform.ensureComment).toHaveBeenCalledTimes(1); + // expect(platform.ensureComment).toHaveBeenCalledWith({ + // content: + // '\n\nIf you accidentally closed this PR, or if you changed your mind: rename this PR to get a fresh replacement PR.', + // topic: 'Renovate Ignore Notification', + // number: 1, + // }); + // }); + + // it('dryrun: skips branch when there is a closed one and add an ignore PR message', async () => { + // GlobalConfig.set({ dryRun: 'full' }); + // platform.findPr.mockResolvedValueOnce(pr); + // platform.getBranchPr.mockResolvedValue(null); + // scm.branchExists.mockResolvedValueOnce(true); + // const res = await checkConfigMigrationBranch(config, migratedData); + // expect(res).toBeNull(); + // expect(logger.info).toHaveBeenCalledWith( + // `DRY-RUN: Would ensure closed PR comment in PR #${pr.number}`, + // ); + // expect(logger.info).toHaveBeenCalledWith( + // 'DRY-RUN: Would delete branch ' + pr.sourceBranch, + // ); + // expect(logger.debug).toHaveBeenCalledWith( + // { prTitle: title }, + // 'Closed PR already exists. Skipping branch.', + // ); + // expect(platform.ensureComment).toHaveBeenCalledTimes(0); + // }); + // }); }); }); diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index c7ed1c5ae1a27a..35de7d4f5b6499 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -28,7 +28,7 @@ export async function checkConfigMigrationBranch( if ( !config.configMigration && - !config.dependencyDashboardChecks?.createConfigMigrationPr + config.dependencyDashboardChecks?.configMigrationInfo === 'unchecked' ) { logger.debug( 'Config migration needed but config migration is disabled and checkbox not ticked.', @@ -41,7 +41,7 @@ export async function checkConfigMigrationBranch( const branchPr = await migrationPrExists( configMigrationBranch, config.baseBranch, - ); // handles open/autoClosed PRs + ); // handles open/autoclosed PRs if (!branchPr) { const commitMessageFactory = new ConfigMigrationCommitMessageFactory( @@ -68,8 +68,7 @@ export async function checkConfigMigrationBranch( // if checkbox is ticked then remove this branch/pr and create new one if ( - !config.dependencyDashboardChecks?.createConfigMigrationPr || - config.dependencyDashboardChecks?.createConfigMigrationPr === 'no' + config.dependencyDashboardChecks?.configMigrationInfo === 'unchecked' ) { logger.debug( 'Closed PR and config migration enabled. Adding checkbox to DD, will check in next run.', diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 7c868680b90523..7392947f93e39e 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -48,8 +48,20 @@ function checkRebaseAll(issueBody: string): boolean { return issueBody.includes(' - [x] '); } -function checkCreateConfigMigrationPr(issueBody: string): boolean { - return issueBody.includes(' - [x] '); +function checkCreateConfigMigrationPr(issueBody: string): string { + if (issueBody.includes('Config Migration necessary.')) { + if (issueBody.includes(' - [x] ')) { + return 'checked'; + } + + if (issueBody.includes(' - [ ] ')) { + return 'unchecked'; + } + + return 'migration-pr-exists'; + } + + return 'unchecked'; } function selectAllRelevantBranches(issueBody: string): string[] { @@ -97,8 +109,8 @@ function parseDashboardIssue(issueBody: string): DependencyDashboard { const dependencyDashboardAllPending = checkApproveAllPendingPR(issueBody); const dependencyDashboardAllRateLimited = checkOpenAllRateLimitedPR(issueBody); - dependencyDashboardChecks['createConfigMigrationPr'] = - checkCreateConfigMigrationPr(issueBody) ? 'yes' : 'no'; + dependencyDashboardChecks['configMigrationInfo'] = + checkCreateConfigMigrationPr(issueBody); return { dependencyDashboardChecks, dependencyDashboardRebaseAllOpen, From 5325b732655787df6fac36d5dbf0f5584110581a Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Fri, 30 Aug 2024 19:58:57 +0530 Subject: [PATCH 06/31] more tests --- .../config-migration/branch/index.spec.ts | 203 +++++++++++------- .../config-migration/branch/index.ts | 12 +- .../repository/config-migration/index.spec.ts | 13 +- .../repository/config-migration/index.ts | 3 +- .../repository/dependency-dashboard.spec.ts | 135 +++++++++++- .../repository/dependency-dashboard.ts | 2 +- 6 files changed, 274 insertions(+), 94 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.spec.ts b/lib/workers/repository/config-migration/branch/index.spec.ts index 98ef7534ffde5a..ae0de884856e1c 100644 --- a/lib/workers/repository/config-migration/branch/index.spec.ts +++ b/lib/workers/repository/config-migration/branch/index.spec.ts @@ -4,7 +4,7 @@ import type { RenovateConfig } from '../../../../../test/util'; import { git, mockedFunction, - // partial, + partial, platform, scm, } from '../../../../../test/util'; @@ -60,7 +60,7 @@ describe('workers/repository/config-migration/branch/index', () => { ), ).resolves.toMatchObject({ result: 'add-checkbox' }); expect(logger.debug).toHaveBeenCalledWith( - 'Config migration needed but config migration is disabled and checkbox not ticked.', + 'Config migration needed but config migration is disabled and checkbox not ticked or not present.', ); }); @@ -127,7 +127,7 @@ describe('workers/repository/config-migration/branch/index', () => { ...config, configMigration: true, dependencyDashboardChecks: { - configMigrationInfo: 'unchecked', + configMigrationInfo: 'no-checkbox', }, }, migratedData, @@ -151,7 +151,7 @@ describe('workers/repository/config-migration/branch/index', () => { ...config, configMigration: true, dependencyDashboardChecks: { - configMigrationInfo: 'unchecked', + configMigrationInfo: 'migration-pr-exists', }, }, migratedData, @@ -174,87 +174,138 @@ describe('workers/repository/config-migration/branch/index', () => { }); platform.getBranchPr.mockResolvedValueOnce(mock()); mockedFunction(rebaseMigrationBranch).mockResolvedValueOnce('committed'); - const res = await checkConfigMigrationBranch(config, migratedData); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: true, + dependencyDashboardChecks: { + configMigrationInfo: 'migration-pr-exists', + }, + }, + migratedData, + ); // TODO: types (#22198) - expect(res).toBe(`${config.branchPrefix!}migrate-config`); + expect(res).toMatchObject({ + result: 'pr-exists', + migrationBranch: `${config.branchPrefix!}migrate-config`, + }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); expect(git.commitFiles).toHaveBeenCalledTimes(0); }); - // it('Creates migration PR', async () => { - // mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( - // 'committed', - // ); - // const res = await checkConfigMigrationBranch(config, migratedData); - // // TODO: types (#22198) - // expect(res).toBe(`${config.branchPrefix!}migrate-config`); - // expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); - // expect(git.commitFiles).toHaveBeenCalledTimes(0); - // expect(logger.debug).toHaveBeenCalledWith('Need to create migration PR'); - // }); + it('Dry runs create migration PR', async () => { + GlobalConfig.set({ + dryRun: 'full', + }); + mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( + 'committed', + ); + const res = await checkConfigMigrationBranch( + { + ...config, + dependencyDashboardChecks: { + configMigrationInfo: 'checked', + }, + }, + migratedData, + ); + // TODO: types (#22198) + expect(res).toMatchObject({ + result: 'pr-exists', + migrationBranch: `${config.branchPrefix!}migrate-config`, + }); + expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); + expect(git.commitFiles).toHaveBeenCalledTimes(0); + }); - // it('Dry runs create migration PR', async () => { - // GlobalConfig.set({ - // dryRun: 'full', - // }); - // mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( - // 'committed', - // ); - // const res = await checkConfigMigrationBranch(config, migratedData); - // // TODO: types (#22198) - // expect(res).toBe(`${config.branchPrefix!}migrate-config`); - // expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); - // expect(git.commitFiles).toHaveBeenCalledTimes(0); - // }); + describe('handle closed PR', () => { + const title = 'PR title'; + const pr = partial({ title, state: 'closed', number: 1 }); - // describe('handle closed PR', () => { - // const title = 'PR title'; - // const pr = partial({ title, state: 'closed', number: 1 }); + // returns result:add-checkbox when migration disabled and closed migration pr exists and checkbox not ticked + it('adds a checkbox when migration is disabled but needed and a closed pr exists', async () => { + platform.findPr.mockResolvedValueOnce(pr); + platform.getBranchPr.mockResolvedValue(null); + scm.branchExists.mockResolvedValueOnce(true); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: false, + dependencyDashboardChecks: { configMigrationInfo: 'no-checkbox' }, + }, + migratedData, + ); + expect(res).toMatchObject({ + result: 'add-checkbox', + }); + }); - // returns result:add-checkbox when migration disabled and closed migration pr exists and checkbox not ticked - // return result: add-checkbox, when migration enabled but closed pr exists + it('deletes old branch and creates new migration branch when migration is disabled but needed, a closed pr exists and checkbox is checked', async () => { + platform.findPr.mockResolvedValueOnce(pr); + platform.getBranchPr.mockResolvedValue(null); + scm.branchExists.mockResolvedValueOnce(true); + mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( + 'committed', + ); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: false, + dependencyDashboardChecks: { configMigrationInfo: 'checked' }, + }, + migratedData, + ); + expect(scm.deleteBranch).toHaveBeenCalledTimes(1); + expect(res).toMatchObject({ + result: 'pr-exists', + migrationBranch: `${config.branchPrefix!}migrate-config`, + }); + expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); + }); - // it('skips branch when there is a closed one delete it and add an ignore PR message', async () => { - // platform.findPr.mockResolvedValueOnce(pr); - // platform.getBranchPr.mockResolvedValue(null); - // scm.branchExists.mockResolvedValueOnce(true); - // const res = await checkConfigMigrationBranch(config, migratedData); - // expect(res).toBeNull(); - // expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); - // expect(scm.commitAndPush).toHaveBeenCalledTimes(0); - // expect(scm.deleteBranch).toHaveBeenCalledTimes(1); - // expect(logger.debug).toHaveBeenCalledWith( - // { prTitle: title }, - // 'Closed PR already exists. Skipping branch.', - // ); - // expect(platform.ensureComment).toHaveBeenCalledTimes(1); - // expect(platform.ensureComment).toHaveBeenCalledWith({ - // content: - // '\n\nIf you accidentally closed this PR, or if you changed your mind: rename this PR to get a fresh replacement PR.', - // topic: 'Renovate Ignore Notification', - // number: 1, - // }); - // }); + // return result: add-checkbox, when migration enabled but closed pr exists + it('adds a checkbox when migration is enabled and a closed pr exists', async () => { + platform.findPr.mockResolvedValueOnce(pr); + platform.getBranchPr.mockResolvedValue(null); + scm.branchExists.mockResolvedValueOnce(true); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: true, + dependencyDashboardChecks: { configMigrationInfo: 'no-checkbox' }, + }, + migratedData, + ); + expect(res).toMatchObject({ + result: 'add-checkbox', + }); + }); - // it('dryrun: skips branch when there is a closed one and add an ignore PR message', async () => { - // GlobalConfig.set({ dryRun: 'full' }); - // platform.findPr.mockResolvedValueOnce(pr); - // platform.getBranchPr.mockResolvedValue(null); - // scm.branchExists.mockResolvedValueOnce(true); - // const res = await checkConfigMigrationBranch(config, migratedData); - // expect(res).toBeNull(); - // expect(logger.info).toHaveBeenCalledWith( - // `DRY-RUN: Would ensure closed PR comment in PR #${pr.number}`, - // ); - // expect(logger.info).toHaveBeenCalledWith( - // 'DRY-RUN: Would delete branch ' + pr.sourceBranch, - // ); - // expect(logger.debug).toHaveBeenCalledWith( - // { prTitle: title }, - // 'Closed PR already exists. Skipping branch.', - // ); - // expect(platform.ensureComment).toHaveBeenCalledTimes(0); - // }); - // }); + it('dry run:deletes old branch and creates new migration branch when migration is disabled but needed, a closed pr exists and checkbox is checked', async () => { + GlobalConfig.set({ + dryRun: 'full', + }); + platform.findPr.mockResolvedValueOnce(pr); + platform.getBranchPr.mockResolvedValue(null); + scm.branchExists.mockResolvedValueOnce(true); + mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( + 'committed', + ); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: false, + dependencyDashboardChecks: { configMigrationInfo: 'checked' }, + }, + migratedData, + ); + expect(scm.deleteBranch).toHaveBeenCalledTimes(0); + expect(res).toMatchObject({ + result: 'pr-exists', + migrationBranch: `${config.branchPrefix!}migrate-config`, + }); + expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); + }); + }); }); }); diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index 35de7d4f5b6499..96efaa462a2971 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -1,3 +1,4 @@ +import type { ConfigMigrationResult } from '..'; import { GlobalConfig } from '../../../../config/global'; import type { RenovateConfig } from '../../../../config/types'; import { logger } from '../../../../logger'; @@ -10,10 +11,8 @@ import { createConfigMigrationBranch } from './create'; import type { MigratedData } from './migrated-data'; import { rebaseMigrationBranch } from './rebase'; -interface CheckConfigMigrationBranchResult { +interface CheckConfigMigrationBranchResult extends ConfigMigrationResult { migrationBranch?: string; - result?: 'add-checkbox' | 'pr-exists'; - prNumber?: number; } export async function checkConfigMigrationBranch( @@ -28,10 +27,11 @@ export async function checkConfigMigrationBranch( if ( !config.configMigration && - config.dependencyDashboardChecks?.configMigrationInfo === 'unchecked' + (config.dependencyDashboardChecks?.configMigrationInfo === 'no-checkbox' || + config.dependencyDashboardChecks?.configMigrationInfo === 'unchecked') ) { logger.debug( - 'Config migration needed but config migration is disabled and checkbox not ticked.', + 'Config migration needed but config migration is disabled and checkbox not ticked or not present.', ); return { result: 'add-checkbox' }; } @@ -68,6 +68,8 @@ export async function checkConfigMigrationBranch( // if checkbox is ticked then remove this branch/pr and create new one if ( + config.dependencyDashboardChecks?.configMigrationInfo === + 'no-checkbox' || config.dependencyDashboardChecks?.configMigrationInfo === 'unchecked' ) { logger.debug( diff --git a/lib/workers/repository/config-migration/index.spec.ts b/lib/workers/repository/config-migration/index.spec.ts index 3067573bedbeb8..58e8ababd8b7b8 100644 --- a/lib/workers/repository/config-migration/index.spec.ts +++ b/lib/workers/repository/config-migration/index.spec.ts @@ -35,16 +35,11 @@ describe('workers/repository/config-migration/index', () => { expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); }); - it('does nothing when config migration is disabled', async () => { - await configMigration({ ...config, configMigration: false }, []); - expect(MigratedDataFactory.getAsync).toHaveBeenCalledTimes(0); - expect(checkConfigMigrationBranch).toHaveBeenCalledTimes(0); - expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); - }); - it('ensures config migration PR when migrated', async () => { const branchList: string[] = []; - mockedFunction(checkConfigMigrationBranch).mockResolvedValue(branchName); + mockedFunction(checkConfigMigrationBranch).mockResolvedValue({ + migrationBranch: branchName, + }); await configMigration(config, branchList); expect(branchList).toContainEqual(branchName); expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(1); @@ -52,7 +47,7 @@ describe('workers/repository/config-migration/index', () => { it('skips pr creation when migration is not needed', async () => { const branchList: string[] = []; - mockedFunction(checkConfigMigrationBranch).mockResolvedValue(null); + mockedFunction(checkConfigMigrationBranch).mockResolvedValue({}); await configMigration(config, branchList); expect(checkConfigMigrationBranch).toHaveBeenCalledTimes(1); expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); diff --git a/lib/workers/repository/config-migration/index.ts b/lib/workers/repository/config-migration/index.ts index 6b493fef33a2b3..a3db5c864094e1 100644 --- a/lib/workers/repository/config-migration/index.ts +++ b/lib/workers/repository/config-migration/index.ts @@ -6,7 +6,6 @@ import { MigratedDataFactory } from './branch/migrated-data'; import { ensureConfigMigrationPr } from './pr'; export interface ConfigMigrationResult { - migrationBranch?: string; result?: 'add-checkbox' | 'pr-exists'; prNumber?: number; } @@ -33,5 +32,5 @@ export async function configMigration( } MigratedDataFactory.reset(); - return { migrationBranch, result, prNumber: pr?.number ?? prNumber }; + return { result, prNumber: pr?.number ?? prNumber }; } diff --git a/lib/workers/repository/dependency-dashboard.spec.ts b/lib/workers/repository/dependency-dashboard.spec.ts index 8a0c6edd53bf6b..038d3b56486158 100644 --- a/lib/workers/repository/dependency-dashboard.spec.ts +++ b/lib/workers/repository/dependency-dashboard.spec.ts @@ -97,7 +97,7 @@ describe('workers/repository/dependency-dashboard', () => { }); await dependencyDashboard.readDashboardBody(conf); expect(conf).toEqual({ - dependencyDashboardChecks: {}, + dependencyDashboardChecks: { configMigrationInfo: 'no-checkbox' }, dependencyDashboardAllPending: false, dependencyDashboardAllRateLimited: false, dependencyDashboardIssue: 1, @@ -125,6 +125,7 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardAllRateLimited: false, dependencyDashboardChecks: { branchName1: 'approve', + configMigrationInfo: 'no-checkbox', }, dependencyDashboardIssue: 1, dependencyDashboardRebaseAllOpen: true, @@ -150,6 +151,7 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardChecks: { branch1: 'global-config', branch2: 'global-config', + configMigrationInfo: 'no-checkbox', }, dependencyDashboardIssue: 1, dependencyDashboardRebaseAllOpen: false, @@ -174,6 +176,7 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardChecks: { branchName1: 'approve', branchName2: 'approve', + configMigrationInfo: 'no-checkbox', }, dependencyDashboardIssue: 1, dependencyDashboardRebaseAllOpen: false, @@ -200,6 +203,7 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardChecks: { branchName5: 'unlimit', branchName6: 'unlimit', + configMigrationInfo: 'no-checkbox', }, dependencyDashboardIssue: 1, dependencyDashboardRebaseAllOpen: false, @@ -210,6 +214,48 @@ describe('workers/repository/dependency-dashboard', () => { }); }); + it('reads dashboard body and config migration checkbox - checked', async () => { + const conf: RenovateConfig = {}; + conf.prCreation = 'approval'; + platform.findIssue.mockResolvedValueOnce({ + title: '', + number: 1, + body: '\n\n - [x] Config Migration necessary.', + }); + await dependencyDashboard.readDashboardBody(conf); + expect(conf.dependencyDashboardChecks).toEqual({ + configMigrationInfo: 'checked', + }); + }); + + it('reads dashboard body and config migration checkbox - unchecked', async () => { + const conf: RenovateConfig = {}; + conf.prCreation = 'approval'; + platform.findIssue.mockResolvedValueOnce({ + title: '', + number: 1, + body: '\n\n - [ ] Config Migration necessary.', + }); + await dependencyDashboard.readDashboardBody(conf); + expect(conf.dependencyDashboardChecks).toEqual({ + configMigrationInfo: 'unchecked', + }); + }); + + it('reads dashboard body and config migration pr link', async () => { + const conf: RenovateConfig = {}; + conf.prCreation = 'approval'; + platform.findIssue.mockResolvedValueOnce({ + title: '', + number: 1, + body: '\n\n Config Migration necessary. You can view the Config Migration PR here #3', + }); + await dependencyDashboard.readDashboardBody(conf); + expect(conf.dependencyDashboardChecks).toEqual({ + configMigrationInfo: 'migration-pr-exists', + }); + }); + it('does not read dashboard body but applies checkedBranches regardless', async () => { const conf: RenovateConfig = {}; conf.dependencyDashboard = false; @@ -616,6 +662,93 @@ describe('workers/repository/dependency-dashboard', () => { await dryRun(branches, platform, 0, 1); }); + it('adds a checkbox for config migration', async () => { + const branches: BranchConfig[] = []; + config.repository = 'test'; + config.packageRules = [ + { + dependencyDashboardApproval: true, + }, + {}, + ]; + config.dependencyDashboardHeader = + 'This is a header for platform:{{platform}}'; + config.dependencyDashboardFooter = + 'And this is a footer for repository:{{repository}}'; + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + undefined, + { + result: 'add-checkbox', + }, + ); + expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); + expect(platform.ensureIssue).toHaveBeenCalledTimes(1); + expect(platform.ensureIssue.mock.calls[0][0].title).toBe( + config.dependencyDashboardTitle, + ); + expect(platform.ensureIssue.mock.calls[0][0].body).toMatch( + ' - [ ] Config Migration necessary.', + ); + }); + + it('adds config migration pr link when it exists', async () => { + const branches: BranchConfig[] = []; + config.repository = 'test'; + config.packageRules = [ + { + dependencyDashboardApproval: true, + }, + {}, + ]; + config.dependencyDashboardHeader = + 'This is a header for platform:{{platform}}'; + config.dependencyDashboardFooter = + 'And this is a footer for repository:{{repository}}'; + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + undefined, + { + result: 'pr-exists', + prNumber: 1, + }, + ); + expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); + expect(platform.ensureIssue).toHaveBeenCalledTimes(1); + expect(platform.ensureIssue.mock.calls[0][0].title).toBe( + config.dependencyDashboardTitle, + ); + expect(platform.ensureIssue.mock.calls[0][0].body).toMatch( + 'Config Migration necessary. You can view the Config Migration PR here', + ); + }); + + it('does not add a config migration checkbox when not needed', async () => { + const branches: BranchConfig[] = []; + config.repository = 'test'; + config.packageRules = [ + { + dependencyDashboardApproval: true, + }, + {}, + ]; + config.dependencyDashboardHeader = + 'This is a header for platform:{{platform}}'; + config.dependencyDashboardFooter = + 'And this is a footer for repository:{{repository}}'; + await dependencyDashboard.ensureDependencyDashboard(config, branches); + expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); + expect(platform.ensureIssue).toHaveBeenCalledTimes(1); + expect(platform.ensureIssue.mock.calls[0][0].title).toBe( + config.dependencyDashboardTitle, + ); + expect(platform.ensureIssue.mock.calls[0][0].body).not.toMatch( + 'Config Migration necessary', + ); + }); + it('contains logged problems', async () => { const branches: BranchConfig[] = [ { diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 7392947f93e39e..d4ce9587e07918 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -61,7 +61,7 @@ function checkCreateConfigMigrationPr(issueBody: string): string { return 'migration-pr-exists'; } - return 'unchecked'; + return 'no-checkbox'; } function selectAllRelevantBranches(issueBody: string): string[] { From a1bbfb9123c84a8dcc08b2c50ccc20659baeb13e Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Fri, 30 Aug 2024 20:01:39 +0530 Subject: [PATCH 07/31] cleanup --- lib/workers/repository/config-migration/branch/index.spec.ts | 5 ----- lib/workers/repository/config-migration/branch/index.ts | 4 ---- 2 files changed, 9 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.spec.ts b/lib/workers/repository/config-migration/branch/index.spec.ts index ae0de884856e1c..0516be5802ea13 100644 --- a/lib/workers/repository/config-migration/branch/index.spec.ts +++ b/lib/workers/repository/config-migration/branch/index.spec.ts @@ -47,7 +47,6 @@ describe('workers/repository/config-migration/branch/index', () => { ); }); - // returns result:add-checkbox when migration is disabled but needed or checkbox is not ticked it('returns add checkbox message when migration disabled and checkbox unchecked', async () => { await expect( checkConfigMigrationBranch( @@ -64,7 +63,6 @@ describe('workers/repository/config-migration/branch/index', () => { ); }); - // return result: pr-exists, when migration branch name & pr number when migration disabled & checkbox ticked or open pr exists it('creates migration branch when migration disabled but checkbox checked', async () => { mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( 'committed', @@ -117,7 +115,6 @@ describe('workers/repository/config-migration/branch/index', () => { ); }); - // return result: pr-exists, migrationBranchName & prNum when migration enabled and no pr or open pr exists it('creates migration branch when migration enabled but no pr exists', async () => { mockedFunction(createConfigMigrationBranch).mockResolvedValueOnce( 'committed', @@ -222,7 +219,6 @@ describe('workers/repository/config-migration/branch/index', () => { const title = 'PR title'; const pr = partial({ title, state: 'closed', number: 1 }); - // returns result:add-checkbox when migration disabled and closed migration pr exists and checkbox not ticked it('adds a checkbox when migration is disabled but needed and a closed pr exists', async () => { platform.findPr.mockResolvedValueOnce(pr); platform.getBranchPr.mockResolvedValue(null); @@ -263,7 +259,6 @@ describe('workers/repository/config-migration/branch/index', () => { expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); }); - // return result: add-checkbox, when migration enabled but closed pr exists it('adds a checkbox when migration is enabled and a closed pr exists', async () => { platform.findPr.mockResolvedValueOnce(pr); platform.getBranchPr.mockResolvedValue(null); diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index 96efaa462a2971..57b9c725ec141d 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -63,10 +63,6 @@ export async function checkConfigMigrationBranch( if (closedPr) { logger.debug('Closed config migration PR found.'); - // previously we used to add a comment and skip the branch - // but now we add a checkbox in DD - // if checkbox is ticked then remove this branch/pr and create new one - if ( config.dependencyDashboardChecks?.configMigrationInfo === 'no-checkbox' || From e6aba4b03d95344e0255e9dcf1a653293237d142 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Sat, 31 Aug 2024 07:08:58 +0530 Subject: [PATCH 08/31] fix closed pr logic --- .../repository/config-migration/branch/index.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index 57b9c725ec141d..8a7cf5ae1341b7 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -1,3 +1,4 @@ +import is from '@sindresorhus/is'; import type { ConfigMigrationResult } from '..'; import { GlobalConfig } from '../../../../config/global'; import type { RenovateConfig } from '../../../../config/types'; @@ -27,7 +28,8 @@ export async function checkConfigMigrationBranch( if ( !config.configMigration && - (config.dependencyDashboardChecks?.configMigrationInfo === 'no-checkbox' || + (is.undefined(config.dependencyDashboardChecks?.configMigrationInfo) || + config.dependencyDashboardChecks?.configMigrationInfo === 'no-checkbox' || config.dependencyDashboardChecks?.configMigrationInfo === 'unchecked') ) { logger.debug( @@ -63,11 +65,7 @@ export async function checkConfigMigrationBranch( if (closedPr) { logger.debug('Closed config migration PR found.'); - if ( - config.dependencyDashboardChecks?.configMigrationInfo === - 'no-checkbox' || - config.dependencyDashboardChecks?.configMigrationInfo === 'unchecked' - ) { + if (config.dependencyDashboardChecks?.configMigrationInfo !== 'checked') { logger.debug( 'Closed PR and config migration enabled. Adding checkbox to DD, will check in next run.', ); From 575216faf7505a206dea8cd8c06fa0ddee6d3507 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Sat, 31 Aug 2024 08:14:38 +0530 Subject: [PATCH 09/31] fix failing tests --- lib/workers/repository/config-migration/pr/index.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/workers/repository/config-migration/pr/index.ts b/lib/workers/repository/config-migration/pr/index.ts index e8fb39f0a319c4..7239453af3cb52 100644 --- a/lib/workers/repository/config-migration/pr/index.ts +++ b/lib/workers/repository/config-migration/pr/index.ts @@ -2,7 +2,8 @@ import is from '@sindresorhus/is'; import { GlobalConfig } from '../../../../config/global'; import type { RenovateConfig } from '../../../../config/types'; import { logger } from '../../../../logger'; -import { Pr, platform } from '../../../../modules/platform'; +import type { Pr } from '../../../../modules/platform'; +import { platform } from '../../../../modules/platform'; import { hashBody } from '../../../../modules/platform/pr-body'; import { scm } from '../../../../modules/platform/scm'; import { emojify } from '../../../../util/emoji'; @@ -128,7 +129,7 @@ ${ await scm.deleteBranch(branchName); return null; } - return null; + throw err; } return null; From 2616fc658ca3fd7230bd01a928fe8c7894296aa7 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Wed, 4 Sep 2024 04:11:24 +0530 Subject: [PATCH 10/31] update docs --- .../config-migration/branch/index.spec.ts | 2 +- .../config-migration/branch/index.ts | 33 +++++++++++-------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.spec.ts b/lib/workers/repository/config-migration/branch/index.spec.ts index 0516be5802ea13..4da79d7b69931c 100644 --- a/lib/workers/repository/config-migration/branch/index.spec.ts +++ b/lib/workers/repository/config-migration/branch/index.spec.ts @@ -59,7 +59,7 @@ describe('workers/repository/config-migration/branch/index', () => { ), ).resolves.toMatchObject({ result: 'add-checkbox' }); expect(logger.debug).toHaveBeenCalledWith( - 'Config migration needed but config migration is disabled and checkbox not ticked or not present.', + 'Config migration needed but config migration is disabled and checkbox not checked or not present.', ); }); diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index 8a7cf5ae1341b7..23523e5e4cddca 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -26,16 +26,20 @@ export async function checkConfigMigrationBranch( return {}; } - if ( - !config.configMigration && - (is.undefined(config.dependencyDashboardChecks?.configMigrationInfo) || - config.dependencyDashboardChecks?.configMigrationInfo === 'no-checkbox' || - config.dependencyDashboardChecks?.configMigrationInfo === 'unchecked') - ) { - logger.debug( - 'Config migration needed but config migration is disabled and checkbox not ticked or not present.', - ); - return { result: 'add-checkbox' }; + const configMigrationCheckbox = + config.dependencyDashboardChecks?.configMigrationInfo; + + if (!config.configMigration) { + if ( + is.undefined(configMigrationCheckbox) || + configMigrationCheckbox === 'no-checkbox' || + configMigrationCheckbox === 'unchecked' + ) { + logger.debug( + 'Config migration needed but config migration is disabled and checkbox not checked or not present.', + ); + return { result: 'add-checkbox' }; + } } const configMigrationBranch = getMigrationBranchName(config); @@ -65,15 +69,18 @@ export async function checkConfigMigrationBranch( if (closedPr) { logger.debug('Closed config migration PR found.'); - if (config.dependencyDashboardChecks?.configMigrationInfo !== 'checked') { + // if a closed pr exists and the checkbox for config migration is not checked + // return add-checkbox result so that the checkbox gets added again + // we only want to create a config migration pr if the checkbox is checked + if (configMigrationCheckbox !== 'checked') { logger.debug( - 'Closed PR and config migration enabled. Adding checkbox to DD, will check in next run.', + 'Config migration is enabled and needed. But a closed pr exists and checkbox is not checked. Skipping migration branch creation.', ); return { result: 'add-checkbox' }; } logger.debug( - 'Closed migration PR found and checkbox is ticked so delete this branch and create a new one.', + 'Closed migration PR found and checkbox is checked. Try to delete this old branch and create a new one.', ); await handlePr(config, closedPr); } From c82eab01414c804aaa17238d8dc1406e8fe4b6c0 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Wed, 4 Sep 2024 04:11:35 +0530 Subject: [PATCH 11/31] update docs --- docs/usage/config-migration.md | 47 +++++++++++++++++++++++++++++ docs/usage/configuration-options.md | 6 +--- 2 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 docs/usage/config-migration.md diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md new file mode 100644 index 00000000000000..a82f01973c0490 --- /dev/null +++ b/docs/usage/config-migration.md @@ -0,0 +1,47 @@ +# Config Migration + +Config migration is necessary when Renovate's configuration options are updated. Users can enable automatic config migration pull requests by setting the [configMigration](./configuration-options.md#configmigration) option to `true`. + +While this feature is disabled by default, we strongly recommend enabling it to prevent unexpected behavior due to outdated configurations. Keeping your config up-to-date can significantly reduce debugging time for both users and maintainers when issues arise. + +Here are the possible scenarios related to config migration: + +1. **No config migration needed** + + Renovate takes no action. + +2. **Config migration needed and enabled** + + Renovate creates a Config Migration PR and adds a link to it at the top of the Dependency Dashboard issue (if enabled). + +3. **Config migration needed but disabled** + + If config migration is required but disabled: + + We add a checkbox to the Dependency Dashboard (if enabled). We call this "on-demand config migration". + + The checkbox will appear in your Dependency Dashboard issue as: + + ``` + [ ] Config migration needed. Please tick this checkbox to create an automated Config Migration PR. + ``` + + - When the user checks the box, Renovate creates a config migration PR and replaces the checkbox with a link: + + ``` + Config Migration necessary. You can view the Config Migration PR here #1 + ``` + +4. **Config migration needed, but a closed migration PR exists** + + In this case, regardless of whether config migration is enabled: + + - Renovate adds a checkbox to the Dependency Dashboard issue (if enabled). + - When checked, Renovate: + 1. Deletes the old config migration branch. + 2. Creates a new config migration PR. + 3. Replaces the checkbox with a link to the new PR in the Dependency Dashboard issue. + + +!!! note + For config migration, Renovate's behavior differs from its usual approach to closed PRs. Instead of commenting on the closed PR that no new PR will be created. Instead Renovate adds a checkbox to the Dependency Dashboard issue, allowing users to request a new config migration PR when needed. diff --git a/docs/usage/configuration-options.md b/docs/usage/configuration-options.md index 2d48d244e520c0..30fc6fc750f0ee 100644 --- a/docs/usage/configuration-options.md +++ b/docs/usage/configuration-options.md @@ -578,11 +578,7 @@ After we changed the [`baseBranches`](#basebranches) feature, the Renovate confi When downgrading JSON5 to JSON Renovate may also remove the JSON5 comments. This can happen because Renovate wrongly converts JSON5 to JSON, thus removing the comments. - -!!! note - When you close a config migration PR, Renovate ignores it forever. - This also means that Renovate won't create a config migration PR in future. - If you closed the PR by accident, find the closed PR and re-name the PR title to get a new PR. +For more details see the [config migration documentation](./config-migration.md). ## configWarningReuseIssue From 6a6b658d2c63368cefc9f7a9de55cd28c7c7f05d Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Thu, 5 Sep 2024 07:45:06 +0530 Subject: [PATCH 12/31] Apply suggestions from code review Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com> --- docs/usage/config-migration.md | 58 ++++++++++++++++++++++------- docs/usage/configuration-options.md | 2 +- 2 files changed, 45 insertions(+), 15 deletions(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index a82f01973c0490..83bac111369ce9 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -1,18 +1,47 @@ # Config Migration -Config migration is necessary when Renovate's configuration options are updated. Users can enable automatic config migration pull requests by setting the [configMigration](./configuration-options.md#configmigration) option to `true`. +The developers may need to rename user-facing configuration options. +For example to: -While this feature is disabled by default, we strongly recommend enabling it to prevent unexpected behavior due to outdated configurations. Keeping your config up-to-date can significantly reduce debugging time for both users and maintainers when issues arise. +- Use a better name for a config option, preset or concept +- Make a experimental configuration option officially supported -Here are the possible scenarios related to config migration: +When this happens, the developers write "config migration" code to tell Renovate the old name and the new name. -1. **No config migration needed** +When Renovate runs, it silently swaps old terms for new terms. +By default, Renovate does _not_ update the terms in your Renovate configuration file! - Renovate takes no action. +### Enabling config migration pull requests -2. **Config migration needed and enabled** +Renovate can create a config migration pull request, that updates old terms in your configuration file. +To enable config migration pull request from Renovate, set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. - Renovate creates a Config Migration PR and adds a link to it at the top of the Dependency Dashboard issue (if enabled). +Config migration PRs are disabled by default. +But we _strongly_ recommend you enable config migration PRs, because: + +- the config migration PR "tells" you something changed +- up-to-date terms help you search the Renovate documentation +- up-to-date terms help you, and us, debug problems quicker + +## Config migration scenarios + +The scenarios for config migration are: + +- No config migration needed +- Config migration needed, and enabled +- Config migration needed, but disabled +- Config migration needed, but there is a closed migration PR + +### No config migration needed + +Renovate takes no action. + +### Config migration needed, and enabled + +Renovate will: + +1. Create a Config Migration PR +1. If the Dependency Dashboard issue is enabled: Renovate puts a link to the Config Migration PR on the dashboard 3. **Config migration needed but disabled** @@ -32,15 +61,16 @@ Here are the possible scenarios related to config migration: Config Migration necessary. You can view the Config Migration PR here #1 ``` -4. **Config migration needed, but a closed migration PR exists** +### Config migration needed, but there is a closed migration PR - In this case, regardless of whether config migration is enabled: +In this case, it does not matter if Config Migration is enabled, or not. +Renovate will: - - Renovate adds a checkbox to the Dependency Dashboard issue (if enabled). - - When checked, Renovate: - 1. Deletes the old config migration branch. - 2. Creates a new config migration PR. - 3. Replaces the checkbox with a link to the new PR in the Dependency Dashboard issue. +- Add a checkbox to the Dependency Dashboard issue (if enabled) +- When you select the checkbox on the dashboard, Renovate will: + 1. Delete the _old_ config migration branch + 1. Create a _new_ config migration PR + 1. Replace the checkbox with a link to the _new_ PR in the Dependency Dashboard issue !!! note diff --git a/docs/usage/configuration-options.md b/docs/usage/configuration-options.md index 30fc6fc750f0ee..81f313993f726f 100644 --- a/docs/usage/configuration-options.md +++ b/docs/usage/configuration-options.md @@ -578,7 +578,7 @@ After we changed the [`baseBranches`](#basebranches) feature, the Renovate confi When downgrading JSON5 to JSON Renovate may also remove the JSON5 comments. This can happen because Renovate wrongly converts JSON5 to JSON, thus removing the comments. -For more details see the [config migration documentation](./config-migration.md). +For more details, read the [config migration documentation](./config-migration.md). ## configWarningReuseIssue From 713684cebdd73344426eaef1f00bf126d1917ba7 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Thu, 5 Sep 2024 07:53:22 +0530 Subject: [PATCH 13/31] update docs --- docs/usage/config-migration.md | 38 ++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index 83bac111369ce9..2fc1395fd26d3c 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -43,23 +43,29 @@ Renovate will: 1. Create a Config Migration PR 1. If the Dependency Dashboard issue is enabled: Renovate puts a link to the Config Migration PR on the dashboard -3. **Config migration needed but disabled** +### Config migration needed but disabled - If config migration is required but disabled: +If config migration is needed, but disabled: - We add a checkbox to the Dependency Dashboard (if enabled). We call this "on-demand config migration". +Renovate adds a checkbox to the Dependency Dashboard (if enabled). +This is "on-demand config migration". - The checkbox will appear in your Dependency Dashboard issue as: +The checkbox looks like this: - ``` - [ ] Config migration needed. Please tick this checkbox to create an automated Config Migration PR. - ``` +``` +- [ ] Config migration needed. Select this checkbox to let Renovate create an automated Config Migration PR. +``` - - When the user checks the box, Renovate creates a config migration PR and replaces the checkbox with a link: +When you select the checkbox: - ``` - Config Migration necessary. You can view the Config Migration PR here #1 - ``` +Renovate creates a config migration PR. +Renovate replaces the checkbox with text with a link to the Config Migration PR. + +For example: + +``` +Config Migration needed. See Config Migration PR: #1. +``` ### Config migration needed, but there is a closed migration PR @@ -68,10 +74,6 @@ Renovate will: - Add a checkbox to the Dependency Dashboard issue (if enabled) - When you select the checkbox on the dashboard, Renovate will: - 1. Delete the _old_ config migration branch - 1. Create a _new_ config migration PR - 1. Replace the checkbox with a link to the _new_ PR in the Dependency Dashboard issue - - -!!! note - For config migration, Renovate's behavior differs from its usual approach to closed PRs. Instead of commenting on the closed PR that no new PR will be created. Instead Renovate adds a checkbox to the Dependency Dashboard issue, allowing users to request a new config migration PR when needed. + 1. Delete the _old_ config migration branch + 1. Create a _new_ config migration PR + 1. Replace the checkbox with a link to the _new_ PR in the Dependency Dashboard issue From 21e75ffbad3e0499910877fc93d6d88844fc1262 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Thu, 5 Sep 2024 08:15:39 +0530 Subject: [PATCH 14/31] apply suggestions --- lib/workers/repository/dependency-dashboard.spec.ts | 12 ++++++------ lib/workers/repository/dependency-dashboard.ts | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/workers/repository/dependency-dashboard.spec.ts b/lib/workers/repository/dependency-dashboard.spec.ts index 038d3b56486158..e60eaa26688c63 100644 --- a/lib/workers/repository/dependency-dashboard.spec.ts +++ b/lib/workers/repository/dependency-dashboard.spec.ts @@ -220,7 +220,7 @@ describe('workers/repository/dependency-dashboard', () => { platform.findIssue.mockResolvedValueOnce({ title: '', number: 1, - body: '\n\n - [x] Config Migration necessary.', + body: '\n\n - [x] Config Migration needed.', }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ @@ -234,7 +234,7 @@ describe('workers/repository/dependency-dashboard', () => { platform.findIssue.mockResolvedValueOnce({ title: '', number: 1, - body: '\n\n - [ ] Config Migration necessary.', + body: '\n\n - [ ] Config Migration needed.', }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ @@ -248,7 +248,7 @@ describe('workers/repository/dependency-dashboard', () => { platform.findIssue.mockResolvedValueOnce({ title: '', number: 1, - body: '\n\n Config Migration necessary. You can view the Config Migration PR here #3', + body: '\n\n Config Migration needed. See Config Migration PR: #3', }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ @@ -689,7 +689,7 @@ describe('workers/repository/dependency-dashboard', () => { config.dependencyDashboardTitle, ); expect(platform.ensureIssue.mock.calls[0][0].body).toMatch( - ' - [ ] Config Migration necessary.', + ' - [ ] Config Migration needed.', ); }); @@ -721,7 +721,7 @@ describe('workers/repository/dependency-dashboard', () => { config.dependencyDashboardTitle, ); expect(platform.ensureIssue.mock.calls[0][0].body).toMatch( - 'Config Migration necessary. You can view the Config Migration PR here', + 'Config Migration needed. See Config Migration PR:', ); }); @@ -745,7 +745,7 @@ describe('workers/repository/dependency-dashboard', () => { config.dependencyDashboardTitle, ); expect(platform.ensureIssue.mock.calls[0][0].body).not.toMatch( - 'Config Migration necessary', + 'Config Migration needed', ); }); diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index d4ce9587e07918..6aa92564746b16 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -49,7 +49,7 @@ function checkRebaseAll(issueBody: string): boolean { } function checkCreateConfigMigrationPr(issueBody: string): string { - if (issueBody.includes('Config Migration necessary.')) { + if (issueBody.includes('Config Migration needed.')) { if (issueBody.includes(' - [x] ')) { return 'checked'; } @@ -285,10 +285,10 @@ export async function ensureDependencyDashboard( let issueBody = ''; if (configMigrationRes?.result === 'pr-exists') { - issueBody += `Config Migration necessary. You can view the Config Migration PR here #${configMigrationRes.prNumber}\n\n`; + issueBody += `Config Migration needed. See Config Migration PR: #${configMigrationRes.prNumber}\n\n`; } else if (configMigrationRes?.result === 'add-checkbox') { issueBody += - ' - [ ] Config Migration necessary. Please tick this checkbox to create an automated Config Migration PR' + + ' - [ ] Config Migration needed. Select this checkbox to let Renovate create an automated Config Migration PR.' + '\n\n'; } From 3d49fd1ffc1c6674deba6c9299006397e749c769 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Thu, 5 Sep 2024 08:18:05 +0530 Subject: [PATCH 15/31] end with full-stop --- lib/workers/repository/dependency-dashboard.spec.ts | 2 +- lib/workers/repository/dependency-dashboard.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/workers/repository/dependency-dashboard.spec.ts b/lib/workers/repository/dependency-dashboard.spec.ts index e60eaa26688c63..6eecd247eb29f6 100644 --- a/lib/workers/repository/dependency-dashboard.spec.ts +++ b/lib/workers/repository/dependency-dashboard.spec.ts @@ -248,7 +248,7 @@ describe('workers/repository/dependency-dashboard', () => { platform.findIssue.mockResolvedValueOnce({ title: '', number: 1, - body: '\n\n Config Migration needed. See Config Migration PR: #3', + body: '\n\n Config Migration needed. See Config Migration PR: #3.', }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 6aa92564746b16..a06d36a69e8d13 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -285,7 +285,7 @@ export async function ensureDependencyDashboard( let issueBody = ''; if (configMigrationRes?.result === 'pr-exists') { - issueBody += `Config Migration needed. See Config Migration PR: #${configMigrationRes.prNumber}\n\n`; + issueBody += `Config Migration needed. See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; } else if (configMigrationRes?.result === 'add-checkbox') { issueBody += ' - [ ] Config Migration needed. Select this checkbox to let Renovate create an automated Config Migration PR.' + From 6d400d9181a3f25ffce48ea7bdf04403cd1652f1 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Thu, 5 Sep 2024 08:20:35 +0530 Subject: [PATCH 16/31] fix lint issues --- docs/usage/config-migration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index 2fc1395fd26d3c..fff08001ca66e5 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -11,7 +11,7 @@ When this happens, the developers write "config migration" code to tell Renovate When Renovate runs, it silently swaps old terms for new terms. By default, Renovate does _not_ update the terms in your Renovate configuration file! -### Enabling config migration pull requests +## Enabling config migration pull requests Renovate can create a config migration pull request, that updates old terms in your configuration file. To enable config migration pull request from Renovate, set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. From f2d27871207c776fa2af3c5fadff0e58fa5c3f71 Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Tue, 10 Sep 2024 05:21:11 +0530 Subject: [PATCH 17/31] Apply suggestions from code review Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com> --- docs/usage/config-migration.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index fff08001ca66e5..e0340662df052a 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -14,7 +14,7 @@ By default, Renovate does _not_ update the terms in your Renovate configuration ## Enabling config migration pull requests Renovate can create a config migration pull request, that updates old terms in your configuration file. -To enable config migration pull request from Renovate, set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. +To get config migration pull requests from Renovate: set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. Config migration PRs are disabled by default. But we _strongly_ recommend you enable config migration PRs, because: @@ -43,7 +43,7 @@ Renovate will: 1. Create a Config Migration PR 1. If the Dependency Dashboard issue is enabled: Renovate puts a link to the Config Migration PR on the dashboard -### Config migration needed but disabled +### Config migration needed, but disabled If config migration is needed, but disabled: @@ -59,7 +59,7 @@ The checkbox looks like this: When you select the checkbox: Renovate creates a config migration PR. -Renovate replaces the checkbox with text with a link to the Config Migration PR. +Renovate replaces the checkbox with a link to the Config Migration PR. For example: From ba213f38b5876d861f5e3c23783247d13b3fb7e2 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Tue, 10 Sep 2024 07:01:33 +0200 Subject: [PATCH 18/31] Update config-migration.md --- docs/usage/config-migration.md | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index e0340662df052a..50e8a8461163a7 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -1,19 +1,20 @@ # Config Migration -The developers may need to rename user-facing configuration options. -For example to: +Renovate maintainers often need to rename, remove or combine configuration options to improve the user experience and mature the product. -- Use a better name for a config option, preset or concept -- Make a experimental configuration option officially supported +When they do so, "config migration" code is added at the same time so that any "legacy" config from users continues to work. +Config migration works by migrating legacy config internally before the config is used. +If done right, it "just works" silently and legacy configs continue working indefinitely. +The only sign when this is necessary is a debug message in logs noting the old and newly migrated configs. -When this happens, the developers write "config migration" code to tell Renovate the old name and the new name. - -When Renovate runs, it silently swaps old terms for new terms. -By default, Renovate does _not_ update the terms in your Renovate configuration file! +By default, none of these changes are applied to Renovate config files (e.g. `renovate.json`) ## Enabling config migration pull requests -Renovate can create a config migration pull request, that updates old terms in your configuration file. +Although legacy config should continue to "just work", it's better for users if their config file uses the latest/correct configuration names. +Using the latest names makes it easier to understand the config and look up documentation for it + +Renovate can create a config migration pull request, that migrates legacy config in your configuration file. To get config migration pull requests from Renovate: set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. Config migration PRs are disabled by default. @@ -30,7 +31,7 @@ The scenarios for config migration are: - No config migration needed - Config migration needed, and enabled - Config migration needed, but disabled -- Config migration needed, but there is a closed migration PR +- Config migration needed, but there is a previously closed migration PR ### No config migration needed @@ -41,14 +42,12 @@ Renovate takes no action. Renovate will: 1. Create a Config Migration PR -1. If the Dependency Dashboard issue is enabled: Renovate puts a link to the Config Migration PR on the dashboard +1. If the Dependency Dashboard issue is enabled then Renovate puts a link to the Config Migration PR on the dashboard ### Config migration needed, but disabled -If config migration is needed, but disabled: - -Renovate adds a checkbox to the Dependency Dashboard (if enabled). -This is "on-demand config migration". +If config migration is needed, but disabled then Renovate adds a checkbox to the Dependency Dashboard if one exists. +This is known as "on-demand" config migration because migration PRs are only created at the request of the user by ticking the checkbox. The checkbox looks like this: From 1300605cbb896d262a1d89acf8988d0c8a8d3402 Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Tue, 17 Sep 2024 12:45:59 +0530 Subject: [PATCH 19/31] Apply suggestions from code review Co-authored-by: Sergei Zharinov --- .../config-migration/branch/index.spec.ts | 1 - lib/workers/repository/dependency-dashboard.ts | 18 +++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.spec.ts b/lib/workers/repository/config-migration/branch/index.spec.ts index 4da79d7b69931c..60db3c3bd4a417 100644 --- a/lib/workers/repository/config-migration/branch/index.spec.ts +++ b/lib/workers/repository/config-migration/branch/index.spec.ts @@ -37,7 +37,6 @@ describe('workers/repository/config-migration/branch/index', () => { config.branchPrefix = 'some/'; }); - // exists when migration not needed it('exits when migration is not needed', async () => { await expect( checkConfigMigrationBranch(config, null), diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index a06d36a69e8d13..9803961afa9238 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -49,19 +49,19 @@ function checkRebaseAll(issueBody: string): boolean { } function checkCreateConfigMigrationPr(issueBody: string): string { - if (issueBody.includes('Config Migration needed.')) { - if (issueBody.includes(' - [x] ')) { - return 'checked'; - } + if (!issueBody.includes('Config Migration needed.')) { + return 'no-checkbox'; + } - if (issueBody.includes(' - [ ] ')) { - return 'unchecked'; - } + if (issueBody.includes(' - [x] ')) { + return 'checked'; + } - return 'migration-pr-exists'; + if (issueBody.includes(' - [ ] ')) { + return 'unchecked'; } - return 'no-checkbox'; + return 'migration-pr-exists'; } function selectAllRelevantBranches(issueBody: string): string[] { From 09b19d58e0771b6329b89f8831f9f0519765013d Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Tue, 17 Sep 2024 23:56:31 +0530 Subject: [PATCH 20/31] apply suggestions and refactor logic --- .../config-migration/branch/index.spec.ts | 19 +++++------ .../config-migration/branch/index.ts | 13 ++++---- .../repository/config-migration/index.spec.ts | 31 ++++++++++++++--- .../repository/config-migration/index.ts | 33 +++++++++++-------- .../repository/dependency-dashboard.ts | 4 ++- 5 files changed, 65 insertions(+), 35 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.spec.ts b/lib/workers/repository/config-migration/branch/index.spec.ts index 60db3c3bd4a417..f093a089f4263c 100644 --- a/lib/workers/repository/config-migration/branch/index.spec.ts +++ b/lib/workers/repository/config-migration/branch/index.spec.ts @@ -40,7 +40,7 @@ describe('workers/repository/config-migration/branch/index', () => { it('exits when migration is not needed', async () => { await expect( checkConfigMigrationBranch(config, null), - ).resolves.toBeEmptyObject(); + ).resolves.toMatchObject({ result: 'no-migration' }); expect(logger.debug).toHaveBeenCalledWith( 'checkConfigMigrationBranch() Config does not need migration', ); @@ -76,7 +76,7 @@ describe('workers/repository/config-migration/branch/index', () => { migratedData, ), ).resolves.toMatchObject({ - result: 'pr-exists', + result: 'migration-branch-exists', migrationBranch: `${config.branchPrefix!}migrate-config`, }); expect(logger.debug).toHaveBeenCalledWith('Need to create migration PR'); @@ -102,9 +102,8 @@ describe('workers/repository/config-migration/branch/index', () => { ); // TODO: types (#22198) expect(res).toMatchObject({ - result: 'pr-exists', + result: 'migration-branch-exists', migrationBranch: `${config.branchPrefix!}migrate-config`, - prNumber: 1, }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); expect(git.commitFiles).toHaveBeenCalledTimes(0); @@ -130,7 +129,7 @@ describe('workers/repository/config-migration/branch/index', () => { ); // TODO: types (#22198) expect(res).toMatchObject({ - result: 'pr-exists', + result: 'migration-branch-exists', migrationBranch: `${config.branchPrefix!}migrate-config`, }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); @@ -154,7 +153,7 @@ describe('workers/repository/config-migration/branch/index', () => { ); // TODO: types (#22198) expect(res).toMatchObject({ - result: 'pr-exists', + result: 'migration-branch-exists', migrationBranch: `${config.branchPrefix!}migrate-config`, }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); @@ -182,7 +181,7 @@ describe('workers/repository/config-migration/branch/index', () => { ); // TODO: types (#22198) expect(res).toMatchObject({ - result: 'pr-exists', + result: 'migration-branch-exists', migrationBranch: `${config.branchPrefix!}migrate-config`, }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); @@ -207,7 +206,7 @@ describe('workers/repository/config-migration/branch/index', () => { ); // TODO: types (#22198) expect(res).toMatchObject({ - result: 'pr-exists', + result: 'migration-branch-exists', migrationBranch: `${config.branchPrefix!}migrate-config`, }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); @@ -252,7 +251,7 @@ describe('workers/repository/config-migration/branch/index', () => { ); expect(scm.deleteBranch).toHaveBeenCalledTimes(1); expect(res).toMatchObject({ - result: 'pr-exists', + result: 'migration-branch-exists', migrationBranch: `${config.branchPrefix!}migrate-config`, }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); @@ -295,7 +294,7 @@ describe('workers/repository/config-migration/branch/index', () => { ); expect(scm.deleteBranch).toHaveBeenCalledTimes(0); expect(res).toMatchObject({ - result: 'pr-exists', + result: 'migration-branch-exists', migrationBranch: `${config.branchPrefix!}migrate-config`, }); expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index 23523e5e4cddca..8694641f87fa00 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -1,5 +1,4 @@ import is from '@sindresorhus/is'; -import type { ConfigMigrationResult } from '..'; import { GlobalConfig } from '../../../../config/global'; import type { RenovateConfig } from '../../../../config/types'; import { logger } from '../../../../logger'; @@ -12,9 +11,10 @@ import { createConfigMigrationBranch } from './create'; import type { MigratedData } from './migrated-data'; import { rebaseMigrationBranch } from './rebase'; -interface CheckConfigMigrationBranchResult extends ConfigMigrationResult { - migrationBranch?: string; -} +export type CheckConfigMigrationBranchResult = + | { result: 'no-migration' } + | { result: 'add-checkbox' } + | { result: 'migration-branch-exists'; migrationBranch: string }; export async function checkConfigMigrationBranch( config: RenovateConfig, @@ -23,7 +23,7 @@ export async function checkConfigMigrationBranch( logger.debug('checkConfigMigrationBranch()'); if (!migratedConfigData) { logger.debug('checkConfigMigrationBranch() Config does not need migration'); - return {}; + return { result: 'no-migration' }; } const configMigrationCheckbox = @@ -108,8 +108,7 @@ export async function checkConfigMigrationBranch( } return { migrationBranch: configMigrationBranch, - result: 'pr-exists', - prNumber: branchPr?.number, + result: 'migration-branch-exists', }; } diff --git a/lib/workers/repository/config-migration/index.spec.ts b/lib/workers/repository/config-migration/index.spec.ts index 58e8ababd8b7b8..78bb6b0d6012ec 100644 --- a/lib/workers/repository/config-migration/index.spec.ts +++ b/lib/workers/repository/config-migration/index.spec.ts @@ -2,6 +2,7 @@ import type { Indent } from 'detect-indent'; import { Fixtures } from '../../../../test/fixtures'; import { mockedFunction, partial } from '../../../../test/util'; import { getConfig } from '../../../config/defaults'; +import type { Pr } from '../../../modules/platform/types'; import { checkConfigMigrationBranch } from './branch'; import { MigratedDataFactory } from './branch/migrated-data'; import { ensureConfigMigrationPr } from './pr'; @@ -29,7 +30,8 @@ describe('workers/repository/config-migration/index', () => { }); it('does nothing when in silent mode', async () => { - await configMigration({ ...config, mode: 'silent' }, []); + const res = await configMigration({ ...config, mode: 'silent' }, []); + expect(res).toMatchObject({ result: 'no-migration' }); expect(MigratedDataFactory.getAsync).toHaveBeenCalledTimes(0); expect(checkConfigMigrationBranch).toHaveBeenCalledTimes(0); expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); @@ -39,17 +41,38 @@ describe('workers/repository/config-migration/index', () => { const branchList: string[] = []; mockedFunction(checkConfigMigrationBranch).mockResolvedValue({ migrationBranch: branchName, + result: 'migration-branch-exists', }); - await configMigration(config, branchList); + mockedFunction(ensureConfigMigrationPr).mockResolvedValue( + partial({ number: 1 }), + ); + const res = await configMigration(config, branchList); + expect(res).toMatchObject({ result: 'pr-exists', prNumber: 1 }); expect(branchList).toContainEqual(branchName); expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(1); }); it('skips pr creation when migration is not needed', async () => { const branchList: string[] = []; - mockedFunction(checkConfigMigrationBranch).mockResolvedValue({}); - await configMigration(config, branchList); + mockedFunction(checkConfigMigrationBranch).mockResolvedValue({ + result: 'no-migration', + }); + const res = await configMigration(config, branchList); + expect(res).toMatchObject({ result: 'no-migration' }); expect(checkConfigMigrationBranch).toHaveBeenCalledTimes(1); expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); }); + + it('adds a checkbox incase a migration pr exists but is created by a different user', async () => { + const branchList: string[] = []; + mockedFunction(checkConfigMigrationBranch).mockResolvedValue({ + migrationBranch: branchName, + result: 'migration-branch-exists', + }); + mockedFunction(ensureConfigMigrationPr).mockResolvedValue(null); + const res = await configMigration(config, branchList); + expect(res).toMatchObject({ result: 'add-checkbox' }); + expect(branchList).toContainEqual(branchName); + expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(1); + }); }); diff --git a/lib/workers/repository/config-migration/index.ts b/lib/workers/repository/config-migration/index.ts index a3db5c864094e1..cf456c087cd705 100644 --- a/lib/workers/repository/config-migration/index.ts +++ b/lib/workers/repository/config-migration/index.ts @@ -1,14 +1,13 @@ import type { RenovateConfig } from '../../../config/types'; import { logger } from '../../../logger'; -import type { Pr } from '../../../modules/platform'; import { checkConfigMigrationBranch } from './branch'; import { MigratedDataFactory } from './branch/migrated-data'; import { ensureConfigMigrationPr } from './pr'; -export interface ConfigMigrationResult { - result?: 'add-checkbox' | 'pr-exists'; - prNumber?: number; -} +export type ConfigMigrationResult = + | { result: 'no-migration' } + | { result: 'add-checkbox' } + | { result: 'pr-exists'; prNumber: number }; export async function configMigration( config: RenovateConfig, @@ -18,19 +17,27 @@ export async function configMigration( logger.debug( 'Config migration issues are not created, updated or closed when mode=silent', ); - return {}; + return { result: 'no-migration' }; } - let pr: Pr | null = null; const migratedConfigData = await MigratedDataFactory.getAsync(); - const { migrationBranch, result, prNumber } = - await checkConfigMigrationBranch(config, migratedConfigData); // null if migration not needed + const res = await checkConfigMigrationBranch(config, migratedConfigData); + + if (res.result !== 'migration-branch-exists') { + return { result: res.result }; + } - if (migrationBranch) { - branchList.push(migrationBranch); - pr = await ensureConfigMigrationPr(config, migratedConfigData!); + branchList.push(res.migrationBranch); + + const pr = await ensureConfigMigrationPr(config, migratedConfigData!); + + // only happens incase a migration pr was created by different user + // for other cases in which a pr could not be found/created we throw error from within the ensureConfigMigrationPr fn + if (!pr) { + return { result: 'add-checkbox' }; } + MigratedDataFactory.reset(); - return { result, prNumber: pr?.number ?? prNumber }; + return { result: 'pr-exists', prNumber: pr.number }; } diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 9803961afa9238..ae8d57b442fefc 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -48,7 +48,9 @@ function checkRebaseAll(issueBody: string): boolean { return issueBody.includes(' - [x] '); } -function checkCreateConfigMigrationPr(issueBody: string): string { +function checkCreateConfigMigrationPr( + issueBody: string, +): 'no-checkbox' | 'checked' | 'unchecked' | 'migration-pr-exists' { if (!issueBody.includes('Config Migration needed.')) { return 'no-checkbox'; } From dfdf99de2b6e6bd13d5acc9fce269be38f671fc1 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Wed, 18 Sep 2024 12:20:30 +0530 Subject: [PATCH 21/31] add message for modified migration branch --- .../config-migration/branch/index.spec.ts | 51 +++++++++++----- .../config-migration/branch/index.ts | 58 +++++++++++++------ .../config-migration/branch/rebase.spec.ts | 9 --- .../config-migration/branch/rebase.ts | 4 -- .../repository/config-migration/index.spec.ts | 46 +++++++++++---- .../repository/config-migration/index.ts | 26 ++++++--- .../repository/dependency-dashboard.spec.ts | 38 +++++++++++- .../repository/dependency-dashboard.ts | 11 +++- 8 files changed, 174 insertions(+), 69 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.spec.ts b/lib/workers/repository/config-migration/branch/index.spec.ts index f093a089f4263c..4d899107dc58a5 100644 --- a/lib/workers/repository/config-migration/branch/index.spec.ts +++ b/lib/workers/repository/config-migration/branch/index.spec.ts @@ -37,16 +37,7 @@ describe('workers/repository/config-migration/branch/index', () => { config.branchPrefix = 'some/'; }); - it('exits when migration is not needed', async () => { - await expect( - checkConfigMigrationBranch(config, null), - ).resolves.toMatchObject({ result: 'no-migration' }); - expect(logger.debug).toHaveBeenCalledWith( - 'checkConfigMigrationBranch() Config does not need migration', - ); - }); - - it('returns add checkbox message when migration disabled and checkbox unchecked', async () => { + it('does nothing when migration disabled and checkbox unchecked', async () => { await expect( checkConfigMigrationBranch( { @@ -56,7 +47,7 @@ describe('workers/repository/config-migration/branch/index', () => { }, migratedData, ), - ).resolves.toMatchObject({ result: 'add-checkbox' }); + ).resolves.toMatchObject({ result: 'no-migration-branch' }); expect(logger.debug).toHaveBeenCalledWith( 'Config migration needed but config migration is disabled and checkbox not checked or not present.', ); @@ -82,6 +73,36 @@ describe('workers/repository/config-migration/branch/index', () => { expect(logger.debug).toHaveBeenCalledWith('Need to create migration PR'); }); + it('does not create a branch if migration branch is modified', async () => { + platform.getBranchPr.mockResolvedValue( + mock({ + number: 1, + }), + ); + scm.isBranchModified.mockResolvedValueOnce(true); + const res = await checkConfigMigrationBranch( + { + ...config, + configMigration: false, + dependencyDashboardChecks: { + configMigrationInfo: 'migration-pr-exists', + }, + }, + migratedData, + ); + // TODO: types (#22198) + expect(res).toMatchObject({ + result: 'migration-branch-modified', + migrationBranch: `${config.branchPrefix!}migrate-config`, + }); + expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); + expect(git.commitFiles).toHaveBeenCalledTimes(0); + expect(platform.refreshPr).toHaveBeenCalledTimes(0); + expect(logger.debug).toHaveBeenCalledWith( + 'Config Migration branch has been modified. Skipping branch rebase.', + ); + }); + it('updates migration branch & refreshes pr when migration disabled but open pr exists', async () => { platform.getBranchPr.mockResolvedValue( mock({ @@ -217,7 +238,7 @@ describe('workers/repository/config-migration/branch/index', () => { const title = 'PR title'; const pr = partial({ title, state: 'closed', number: 1 }); - it('adds a checkbox when migration is disabled but needed and a closed pr exists', async () => { + it('does not create a branch when migration is disabled but needed and a closed pr exists', async () => { platform.findPr.mockResolvedValueOnce(pr); platform.getBranchPr.mockResolvedValue(null); scm.branchExists.mockResolvedValueOnce(true); @@ -230,7 +251,7 @@ describe('workers/repository/config-migration/branch/index', () => { migratedData, ); expect(res).toMatchObject({ - result: 'add-checkbox', + result: 'no-migration-branch', }); }); @@ -257,7 +278,7 @@ describe('workers/repository/config-migration/branch/index', () => { expect(scm.checkoutBranch).toHaveBeenCalledTimes(1); }); - it('adds a checkbox when migration is enabled and a closed pr exists', async () => { + it('does not create a branch when migration is enabled and a closed pr exists', async () => { platform.findPr.mockResolvedValueOnce(pr); platform.getBranchPr.mockResolvedValue(null); scm.branchExists.mockResolvedValueOnce(true); @@ -270,7 +291,7 @@ describe('workers/repository/config-migration/branch/index', () => { migratedData, ); expect(res).toMatchObject({ - result: 'add-checkbox', + result: 'no-migration-branch', }); }); diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index 8694641f87fa00..91263ab399da63 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -12,20 +12,17 @@ import type { MigratedData } from './migrated-data'; import { rebaseMigrationBranch } from './rebase'; export type CheckConfigMigrationBranchResult = - | { result: 'no-migration' } - | { result: 'add-checkbox' } - | { result: 'migration-branch-exists'; migrationBranch: string }; + | { result: 'no-migration-branch' } + | { + result: 'migration-branch-exists' | 'migration-branch-modified'; + migrationBranch: string; + }; export async function checkConfigMigrationBranch( config: RenovateConfig, - migratedConfigData: MigratedData | null, + migratedConfigData: MigratedData, ): Promise { logger.debug('checkConfigMigrationBranch()'); - if (!migratedConfigData) { - logger.debug('checkConfigMigrationBranch() Config does not need migration'); - return { result: 'no-migration' }; - } - const configMigrationCheckbox = config.dependencyDashboardChecks?.configMigrationInfo; @@ -38,7 +35,7 @@ export async function checkConfigMigrationBranch( logger.debug( 'Config migration needed but config migration is disabled and checkbox not checked or not present.', ); - return { result: 'add-checkbox' }; + return { result: 'no-migration-branch' }; } } @@ -76,7 +73,7 @@ export async function checkConfigMigrationBranch( logger.debug( 'Config migration is enabled and needed. But a closed pr exists and checkbox is not checked. Skipping migration branch creation.', ); - return { result: 'add-checkbox' }; + return { result: 'no-migration-branch' }; } logger.debug( @@ -86,29 +83,44 @@ export async function checkConfigMigrationBranch( } } + let result: + | 'no-migration-branch' + | 'migration-branch-exists' + | 'migration-branch-modified'; + if (branchPr) { logger.debug('Config Migration PR already exists'); - await rebaseMigrationBranch(config, migratedConfigData); - if (platform.refreshPr) { - const configMigrationPr = await platform.getBranchPr( - configMigrationBranch, - config.baseBranch, + + if (await isMigrationBranchModified(config, configMigrationBranch)) { + logger.debug( + 'Config Migration branch has been modified. Skipping branch rebase.', ); - if (configMigrationPr) { - await platform.refreshPr(configMigrationPr.number); + result = 'migration-branch-modified'; + } else { + await rebaseMigrationBranch(config, migratedConfigData); + if (platform.refreshPr) { + const configMigrationPr = await platform.getBranchPr( + configMigrationBranch, + config.baseBranch, + ); + if (configMigrationPr) { + await platform.refreshPr(configMigrationPr.number); + } } + result = 'migration-branch-exists'; } } else { logger.debug('Config Migration PR does not exist'); logger.debug('Need to create migration PR'); await createConfigMigrationBranch(config, migratedConfigData); + result = 'migration-branch-exists'; } if (!GlobalConfig.get('dryRun')) { await scm.checkoutBranch(configMigrationBranch); } return { migrationBranch: configMigrationBranch, - result: 'migration-branch-exists', + result, }; } @@ -128,3 +140,11 @@ async function handlePr(config: RenovateConfig, pr: Pr): Promise { } } } + +async function isMigrationBranchModified( + config: RenovateConfig, + configMigrationBranch: string, +): Promise { + const baseBranch = config.defaultBranch!; + return await scm.isBranchModified(configMigrationBranch, baseBranch); +} diff --git a/lib/workers/repository/config-migration/branch/rebase.spec.ts b/lib/workers/repository/config-migration/branch/rebase.spec.ts index cc8b1573b67576..4f6b9d9019d328 100644 --- a/lib/workers/repository/config-migration/branch/rebase.spec.ts +++ b/lib/workers/repository/config-migration/branch/rebase.spec.ts @@ -50,15 +50,6 @@ describe('workers/repository/config-migration/branch/rebase', () => { }; }); - it('does not rebase modified branch', async () => { - scm.isBranchModified.mockResolvedValueOnce(true); - - await rebaseMigrationBranch(config, migratedConfigData); - - expect(scm.checkoutBranch).toHaveBeenCalledTimes(0); - expect(scm.commitAndPush).toHaveBeenCalledTimes(0); - }); - it.each([ ['renovate.json', renovateConfigJson], ['renovate.json5', renovateConfigJson5], diff --git a/lib/workers/repository/config-migration/branch/rebase.ts b/lib/workers/repository/config-migration/branch/rebase.ts index 994e349e7908d7..32481a4239ad20 100644 --- a/lib/workers/repository/config-migration/branch/rebase.ts +++ b/lib/workers/repository/config-migration/branch/rebase.ts @@ -17,10 +17,6 @@ export async function rebaseMigrationBranch( logger.debug('Checking if migration branch needs rebasing'); const baseBranch = config.defaultBranch!; const branchName = getMigrationBranchName(config); - if (await scm.isBranchModified(branchName, baseBranch)) { - logger.debug('Migration branch has been edited and cannot be rebased'); - return null; - } const configFileName = migratedConfigData.filename; let contents = migratedConfigData.content; const existingContents = await getFile(configFileName, branchName); diff --git a/lib/workers/repository/config-migration/index.spec.ts b/lib/workers/repository/config-migration/index.spec.ts index 78bb6b0d6012ec..19ae2263eeb0f2 100644 --- a/lib/workers/repository/config-migration/index.spec.ts +++ b/lib/workers/repository/config-migration/index.spec.ts @@ -37,7 +37,16 @@ describe('workers/repository/config-migration/index', () => { expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); }); - it('ensures config migration PR when migrated', async () => { + it('skips pr creation when migration is not needed', async () => { + const branchList: string[] = []; + mockedFunction(MigratedDataFactory.getAsync).mockResolvedValue(null); + const res = await configMigration(config, branchList); + expect(res).toMatchObject({ result: 'no-migration' }); + expect(checkConfigMigrationBranch).toHaveBeenCalledTimes(0); + expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); + }); + + it('creates migration pr if needed', async () => { const branchList: string[] = []; mockedFunction(checkConfigMigrationBranch).mockResolvedValue({ migrationBranch: branchName, @@ -52,27 +61,44 @@ describe('workers/repository/config-migration/index', () => { expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(1); }); - it('skips pr creation when migration is not needed', async () => { + it('returns add-checkbox if migration pr exists but is created by another user', async () => { const branchList: string[] = []; mockedFunction(checkConfigMigrationBranch).mockResolvedValue({ - result: 'no-migration', + migrationBranch: branchName, + result: 'migration-branch-exists', }); + mockedFunction(ensureConfigMigrationPr).mockResolvedValue(null); const res = await configMigration(config, branchList); - expect(res).toMatchObject({ result: 'no-migration' }); - expect(checkConfigMigrationBranch).toHaveBeenCalledTimes(1); - expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); + expect(res).toMatchObject({ result: 'add-checkbox' }); + expect(branchList).toContainEqual(branchName); + expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(1); }); - it('adds a checkbox incase a migration pr exists but is created by a different user', async () => { + it('returns pr-modified incase the migration pr has been modified', async () => { const branchList: string[] = []; mockedFunction(checkConfigMigrationBranch).mockResolvedValue({ migrationBranch: branchName, - result: 'migration-branch-exists', + result: 'migration-branch-modified', }); - mockedFunction(ensureConfigMigrationPr).mockResolvedValue(null); + mockedFunction(ensureConfigMigrationPr).mockResolvedValue( + partial({ + number: 1, + }), + ); const res = await configMigration(config, branchList); - expect(res).toMatchObject({ result: 'add-checkbox' }); + expect(res).toMatchObject({ result: 'pr-modified', prNumber: 1 }); expect(branchList).toContainEqual(branchName); expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(1); }); + + it('returns add-checkbox if migration is needed but not demanded', async () => { + const branchList: string[] = []; + mockedFunction(checkConfigMigrationBranch).mockResolvedValue({ + result: 'no-migration-branch', + }); + const res = await configMigration(config, branchList); + expect(res).toMatchObject({ result: 'add-checkbox' }); + expect(branchList).toBeEmptyArray(); + expect(ensureConfigMigrationPr).toHaveBeenCalledTimes(0); + }); }); diff --git a/lib/workers/repository/config-migration/index.ts b/lib/workers/repository/config-migration/index.ts index cf456c087cd705..c3bfe2b4d5054d 100644 --- a/lib/workers/repository/config-migration/index.ts +++ b/lib/workers/repository/config-migration/index.ts @@ -7,7 +7,7 @@ import { ensureConfigMigrationPr } from './pr'; export type ConfigMigrationResult = | { result: 'no-migration' } | { result: 'add-checkbox' } - | { result: 'pr-exists'; prNumber: number }; + | { result: 'pr-exists' | 'pr-modified'; prNumber: number }; export async function configMigration( config: RenovateConfig, @@ -15,29 +15,41 @@ export async function configMigration( ): Promise { if (config.mode === 'silent') { logger.debug( - 'Config migration issues are not created, updated or closed when mode=silent', + 'configMigration(): Config migration issues are not created, updated or closed when mode=silent', ); return { result: 'no-migration' }; } const migratedConfigData = await MigratedDataFactory.getAsync(); + if (!migratedConfigData) { + logger.debug('configMigration(): Config does not need migration'); + MigratedDataFactory.reset(); + return { result: 'no-migration' }; + } + const res = await checkConfigMigrationBranch(config, migratedConfigData); - if (res.result !== 'migration-branch-exists') { - return { result: res.result }; + // migration needed but not demanded by user + if (res.result === 'no-migration-branch') { + return { result: 'add-checkbox' }; } branchList.push(res.migrationBranch); - const pr = await ensureConfigMigrationPr(config, migratedConfigData!); + const pr = await ensureConfigMigrationPr(config, migratedConfigData); - // only happens incase a migration pr was created by different user + // only happens incase a migration pr was created by another user // for other cases in which a pr could not be found/created we throw error from within the ensureConfigMigrationPr fn if (!pr) { + // warning already logged within ensureConfigMigrationPr need to log here again return { result: 'add-checkbox' }; } MigratedDataFactory.reset(); - return { result: 'pr-exists', prNumber: pr.number }; + return { + result: + res.result === 'migration-branch-exists' ? 'pr-exists' : 'pr-modified', + prNumber: pr.number, + }; } diff --git a/lib/workers/repository/dependency-dashboard.spec.ts b/lib/workers/repository/dependency-dashboard.spec.ts index 6eecd247eb29f6..8980dda1fabf85 100644 --- a/lib/workers/repository/dependency-dashboard.spec.ts +++ b/lib/workers/repository/dependency-dashboard.spec.ts @@ -689,7 +689,7 @@ describe('workers/repository/dependency-dashboard', () => { config.dependencyDashboardTitle, ); expect(platform.ensureIssue.mock.calls[0][0].body).toMatch( - ' - [ ] Config Migration needed.', + ' - [ ] Select this checkbox to let Renovate create an automated Config Migration PR.', ); }); @@ -721,7 +721,39 @@ describe('workers/repository/dependency-dashboard', () => { config.dependencyDashboardTitle, ); expect(platform.ensureIssue.mock.calls[0][0].body).toMatch( - 'Config Migration needed. See Config Migration PR:', + '## Config Migration Needed\n\nSee Config Migration PR:', + ); + }); + + it('adds related text when config migration pr has been modified', async () => { + const branches: BranchConfig[] = []; + config.repository = 'test'; + config.packageRules = [ + { + dependencyDashboardApproval: true, + }, + {}, + ]; + config.dependencyDashboardHeader = + 'This is a header for platform:{{platform}}'; + config.dependencyDashboardFooter = + 'And this is a footer for repository:{{repository}}'; + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + undefined, + { + result: 'pr-modified', + prNumber: 1, + }, + ); + expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); + expect(platform.ensureIssue).toHaveBeenCalledTimes(1); + expect(platform.ensureIssue.mock.calls[0][0].title).toBe( + config.dependencyDashboardTitle, + ); + expect(platform.ensureIssue.mock.calls[0][0].body).toMatch( + 'The Config Migration branch exists but has been modified by another user. Renovate will not push to this branch unless it is first deleted.', ); }); @@ -745,7 +777,7 @@ describe('workers/repository/dependency-dashboard', () => { config.dependencyDashboardTitle, ); expect(platform.ensureIssue.mock.calls[0][0].body).not.toMatch( - 'Config Migration needed', + '## Config Migration Needed', ); }); diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index ae8d57b442fefc..91bfb4c0795c21 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -287,10 +287,17 @@ export async function ensureDependencyDashboard( let issueBody = ''; if (configMigrationRes?.result === 'pr-exists') { - issueBody += `Config Migration needed. See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; + issueBody += + '## Config Migration Needed\n\n' + + `See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; + } else if (configMigrationRes?.result === 'pr-modified') { + issueBody += + '## Config Migration Needed\n\n' + + `The Config Migration branch exists but has been modified by another user. Renovate will not push to this branch unless it is first deleted. \n\n See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; } else if (configMigrationRes?.result === 'add-checkbox') { issueBody += - ' - [ ] Config Migration needed. Select this checkbox to let Renovate create an automated Config Migration PR.' + + '## Config Migration Needed\n\n' + + ' - [ ] Select this checkbox to let Renovate create an automated Config Migration PR.' + '\n\n'; } From 315a9fd078fed2df15c2e48ea654d5de478f4067 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Fri, 20 Sep 2024 10:21:00 +0530 Subject: [PATCH 22/31] refactor: tidyting up --- docs/usage/config-migration.md | 6 +++--- lib/workers/repository/config-migration/branch/index.ts | 2 +- lib/workers/repository/config-migration/index.ts | 9 +++++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index 50e8a8461163a7..333c31ec93f0e0 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -12,7 +12,7 @@ By default, none of these changes are applied to Renovate config files (e.g. `re ## Enabling config migration pull requests Although legacy config should continue to "just work", it's better for users if their config file uses the latest/correct configuration names. -Using the latest names makes it easier to understand the config and look up documentation for it +Using the latest names makes it easier to understand the config and look up documentation for it. Renovate can create a config migration pull request, that migrates legacy config in your configuration file. To get config migration pull requests from Renovate: set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. @@ -52,7 +52,7 @@ This is known as "on-demand" config migration because migration PRs are only cre The checkbox looks like this: ``` -- [ ] Config migration needed. Select this checkbox to let Renovate create an automated Config Migration PR. +- [ ] Select this checkbox to let Renovate create an automated Config Migration PR. ``` When you select the checkbox: @@ -63,7 +63,7 @@ Renovate replaces the checkbox with a link to the Config Migration PR. For example: ``` -Config Migration needed. See Config Migration PR: #1. +See Config Migration PR: #1. ``` ### Config migration needed, but there is a closed migration PR diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index 91263ab399da63..cb9cbf8e3601cb 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -67,7 +67,7 @@ export async function checkConfigMigrationBranch( logger.debug('Closed config migration PR found.'); // if a closed pr exists and the checkbox for config migration is not checked - // return add-checkbox result so that the checkbox gets added again + // return no-migration-branch result so that the checkbox gets added again // we only want to create a config migration pr if the checkbox is checked if (configMigrationCheckbox !== 'checked') { logger.debug( diff --git a/lib/workers/repository/config-migration/index.ts b/lib/workers/repository/config-migration/index.ts index c3bfe2b4d5054d..2da923acc33830 100644 --- a/lib/workers/repository/config-migration/index.ts +++ b/lib/workers/repository/config-migration/index.ts @@ -15,14 +15,14 @@ export async function configMigration( ): Promise { if (config.mode === 'silent') { logger.debug( - 'configMigration(): Config migration issues are not created, updated or closed when mode=silent', + 'Config migration issues are not created, updated or closed when mode=silent', ); return { result: 'no-migration' }; } const migratedConfigData = await MigratedDataFactory.getAsync(); if (!migratedConfigData) { - logger.debug('configMigration(): Config does not need migration'); + logger.debug('Config does not need migration'); MigratedDataFactory.reset(); return { result: 'no-migration' }; } @@ -31,6 +31,7 @@ export async function configMigration( // migration needed but not demanded by user if (res.result === 'no-migration-branch') { + MigratedDataFactory.reset(); return { result: 'add-checkbox' }; } @@ -39,9 +40,9 @@ export async function configMigration( const pr = await ensureConfigMigrationPr(config, migratedConfigData); // only happens incase a migration pr was created by another user - // for other cases in which a pr could not be found/created we throw error from within the ensureConfigMigrationPr fn + // for other cases in which a pr could not be found/created we log warning and throw error from within the ensureConfigMigrationPr fn if (!pr) { - // warning already logged within ensureConfigMigrationPr need to log here again + MigratedDataFactory.reset(); return { result: 'add-checkbox' }; } From 2ffb2f517999237040e86abc422c5b3d7c2f9306 Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Mon, 23 Sep 2024 11:09:03 +0530 Subject: [PATCH 23/31] Apply suggestions from code review Co-authored-by: Sergei Zharinov --- lib/workers/repository/config-migration/branch/index.ts | 5 +---- lib/workers/repository/dependency-dashboard.ts | 6 +++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index cb9cbf8e3601cb..ac9f1afc38b26d 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -83,10 +83,7 @@ export async function checkConfigMigrationBranch( } } - let result: - | 'no-migration-branch' - | 'migration-branch-exists' - | 'migration-branch-modified'; + let result: CheckConfigMigrationBranchResult['result']; if (branchPr) { logger.debug('Config Migration PR already exists'); diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 91bfb4c0795c21..2dd2a881008116 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -48,7 +48,7 @@ function checkRebaseAll(issueBody: string): boolean { return issueBody.includes(' - [x] '); } -function checkCreateConfigMigrationPr( +function getConfigMigrationCheckboxState( issueBody: string, ): 'no-checkbox' | 'checked' | 'unchecked' | 'migration-pr-exists' { if (!issueBody.includes('Config Migration needed.')) { @@ -199,7 +199,7 @@ export async function ensureDependencyDashboard( config: SelectAllConfig, allBranches: BranchConfig[], packageFiles: Record = {}, - configMigrationRes?: ConfigMigrationResult, + configMigrationRes: ConfigMigrationResult, ): Promise { logger.debug('ensureDependencyDashboard()'); if (config.mode === 'silent') { @@ -286,7 +286,7 @@ export async function ensureDependencyDashboard( } let issueBody = ''; - if (configMigrationRes?.result === 'pr-exists') { + if (configMigrationRes.result === 'pr-exists') { issueBody += '## Config Migration Needed\n\n' + `See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; From edb48c337d94f098c1de492c7f43d4cc2a3c02e4 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Tue, 24 Sep 2024 03:27:42 +0530 Subject: [PATCH 24/31] apply suggestions --- .../config-migration/branch/index.spec.ts | 36 ++- .../config-migration/branch/index.ts | 12 +- .../repository/dependency-dashboard.spec.ts | 223 +++++++++++++++--- .../repository/dependency-dashboard.ts | 4 +- 4 files changed, 218 insertions(+), 57 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/index.spec.ts b/lib/workers/repository/config-migration/branch/index.spec.ts index 4d899107dc58a5..c2b98b4687091f 100644 --- a/lib/workers/repository/config-migration/branch/index.spec.ts +++ b/lib/workers/repository/config-migration/branch/index.spec.ts @@ -43,7 +43,9 @@ describe('workers/repository/config-migration/branch/index', () => { { ...config, configMigration: false, - dependencyDashboardChecks: { configMigrationInfo: 'unchecked' }, + dependencyDashboardChecks: { + configMigrationCheckboxState: 'unchecked', + }, }, migratedData, ), @@ -62,7 +64,9 @@ describe('workers/repository/config-migration/branch/index', () => { { ...config, configMigration: false, - dependencyDashboardChecks: { configMigrationInfo: 'checked' }, + dependencyDashboardChecks: { + configMigrationCheckboxState: 'checked', + }, }, migratedData, ), @@ -85,7 +89,7 @@ describe('workers/repository/config-migration/branch/index', () => { ...config, configMigration: false, dependencyDashboardChecks: { - configMigrationInfo: 'migration-pr-exists', + configMigrationCheckboxState: 'migration-pr-exists', }, }, migratedData, @@ -116,7 +120,7 @@ describe('workers/repository/config-migration/branch/index', () => { ...config, configMigration: false, dependencyDashboardChecks: { - configMigrationInfo: 'migration-pr-exists', + configMigrationCheckboxState: 'migration-pr-exists', }, }, migratedData, @@ -143,7 +147,7 @@ describe('workers/repository/config-migration/branch/index', () => { ...config, configMigration: true, dependencyDashboardChecks: { - configMigrationInfo: 'no-checkbox', + configMigrationCheckboxState: 'no-checkbox', }, }, migratedData, @@ -167,7 +171,7 @@ describe('workers/repository/config-migration/branch/index', () => { ...config, configMigration: true, dependencyDashboardChecks: { - configMigrationInfo: 'migration-pr-exists', + configMigrationCheckboxState: 'migration-pr-exists', }, }, migratedData, @@ -195,7 +199,7 @@ describe('workers/repository/config-migration/branch/index', () => { ...config, configMigration: true, dependencyDashboardChecks: { - configMigrationInfo: 'migration-pr-exists', + configMigrationCheckboxState: 'migration-pr-exists', }, }, migratedData, @@ -220,7 +224,7 @@ describe('workers/repository/config-migration/branch/index', () => { { ...config, dependencyDashboardChecks: { - configMigrationInfo: 'checked', + configMigrationCheckboxState: 'checked', }, }, migratedData, @@ -246,7 +250,9 @@ describe('workers/repository/config-migration/branch/index', () => { { ...config, configMigration: false, - dependencyDashboardChecks: { configMigrationInfo: 'no-checkbox' }, + dependencyDashboardChecks: { + configMigrationCheckboxState: 'no-checkbox', + }, }, migratedData, ); @@ -266,7 +272,9 @@ describe('workers/repository/config-migration/branch/index', () => { { ...config, configMigration: false, - dependencyDashboardChecks: { configMigrationInfo: 'checked' }, + dependencyDashboardChecks: { + configMigrationCheckboxState: 'checked', + }, }, migratedData, ); @@ -286,7 +294,9 @@ describe('workers/repository/config-migration/branch/index', () => { { ...config, configMigration: true, - dependencyDashboardChecks: { configMigrationInfo: 'no-checkbox' }, + dependencyDashboardChecks: { + configMigrationCheckboxState: 'no-checkbox', + }, }, migratedData, ); @@ -309,7 +319,9 @@ describe('workers/repository/config-migration/branch/index', () => { { ...config, configMigration: false, - dependencyDashboardChecks: { configMigrationInfo: 'checked' }, + dependencyDashboardChecks: { + configMigrationCheckboxState: 'checked', + }, }, migratedData, ); diff --git a/lib/workers/repository/config-migration/branch/index.ts b/lib/workers/repository/config-migration/branch/index.ts index ac9f1afc38b26d..3f14a180eecc76 100644 --- a/lib/workers/repository/config-migration/branch/index.ts +++ b/lib/workers/repository/config-migration/branch/index.ts @@ -23,14 +23,14 @@ export async function checkConfigMigrationBranch( migratedConfigData: MigratedData, ): Promise { logger.debug('checkConfigMigrationBranch()'); - const configMigrationCheckbox = - config.dependencyDashboardChecks?.configMigrationInfo; + const configMigrationCheckboxState = + config.dependencyDashboardChecks?.configMigrationCheckboxState; if (!config.configMigration) { if ( - is.undefined(configMigrationCheckbox) || - configMigrationCheckbox === 'no-checkbox' || - configMigrationCheckbox === 'unchecked' + is.undefined(configMigrationCheckboxState) || + configMigrationCheckboxState === 'no-checkbox' || + configMigrationCheckboxState === 'unchecked' ) { logger.debug( 'Config migration needed but config migration is disabled and checkbox not checked or not present.', @@ -69,7 +69,7 @@ export async function checkConfigMigrationBranch( // if a closed pr exists and the checkbox for config migration is not checked // return no-migration-branch result so that the checkbox gets added again // we only want to create a config migration pr if the checkbox is checked - if (configMigrationCheckbox !== 'checked') { + if (configMigrationCheckboxState !== 'checked') { logger.debug( 'Config migration is enabled and needed. But a closed pr exists and checkbox is not checked. Skipping migration branch creation.', ); diff --git a/lib/workers/repository/dependency-dashboard.spec.ts b/lib/workers/repository/dependency-dashboard.spec.ts index 8980dda1fabf85..d35fccf21d8bef 100644 --- a/lib/workers/repository/dependency-dashboard.spec.ts +++ b/lib/workers/repository/dependency-dashboard.spec.ts @@ -78,7 +78,12 @@ async function dryRun( ensureIssueCalls: number, ) { GlobalConfig.set({ dryRun: 'full' }); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes( ensureIssueClosingCalls, ); @@ -97,7 +102,9 @@ describe('workers/repository/dependency-dashboard', () => { }); await dependencyDashboard.readDashboardBody(conf); expect(conf).toEqual({ - dependencyDashboardChecks: { configMigrationInfo: 'no-checkbox' }, + dependencyDashboardChecks: { + configMigrationCheckboxState: 'no-checkbox', + }, dependencyDashboardAllPending: false, dependencyDashboardAllRateLimited: false, dependencyDashboardIssue: 1, @@ -125,7 +132,7 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardAllRateLimited: false, dependencyDashboardChecks: { branchName1: 'approve', - configMigrationInfo: 'no-checkbox', + configMigrationCheckboxState: 'no-checkbox', }, dependencyDashboardIssue: 1, dependencyDashboardRebaseAllOpen: true, @@ -151,7 +158,7 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardChecks: { branch1: 'global-config', branch2: 'global-config', - configMigrationInfo: 'no-checkbox', + configMigrationCheckboxState: 'no-checkbox', }, dependencyDashboardIssue: 1, dependencyDashboardRebaseAllOpen: false, @@ -176,7 +183,7 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardChecks: { branchName1: 'approve', branchName2: 'approve', - configMigrationInfo: 'no-checkbox', + configMigrationCheckboxState: 'no-checkbox', }, dependencyDashboardIssue: 1, dependencyDashboardRebaseAllOpen: false, @@ -203,7 +210,7 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardChecks: { branchName5: 'unlimit', branchName6: 'unlimit', - configMigrationInfo: 'no-checkbox', + configMigrationCheckboxState: 'no-checkbox', }, dependencyDashboardIssue: 1, dependencyDashboardRebaseAllOpen: false, @@ -224,7 +231,7 @@ describe('workers/repository/dependency-dashboard', () => { }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ - configMigrationInfo: 'checked', + configMigrationCheckboxState: 'checked', }); }); @@ -238,7 +245,7 @@ describe('workers/repository/dependency-dashboard', () => { }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ - configMigrationInfo: 'unchecked', + configMigrationCheckboxState: 'unchecked', }); }); @@ -252,7 +259,7 @@ describe('workers/repository/dependency-dashboard', () => { }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ - configMigrationInfo: 'migration-pr-exists', + configMigrationCheckboxState: 'migration-pr-exists', }); }); @@ -285,7 +292,12 @@ describe('workers/repository/dependency-dashboard', () => { it('does nothing if mode=silent', async () => { const branches: BranchConfig[] = []; config.mode = 'silent'; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(0); @@ -295,7 +307,12 @@ describe('workers/repository/dependency-dashboard', () => { it('do nothing if dependencyDashboard is disabled', async () => { const branches: BranchConfig[] = []; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(1); expect(platform.ensureIssue).toHaveBeenCalledTimes(0); @@ -315,7 +332,12 @@ describe('workers/repository/dependency-dashboard', () => { dependencyDashboardApproval: false, }, ]; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(1); expect(platform.ensureIssue).toHaveBeenCalledTimes(0); @@ -327,7 +349,12 @@ describe('workers/repository/dependency-dashboard', () => { const branches: BranchConfig[] = []; config.dependencyDashboard = true; config.dependencyDashboardAutoclose = true; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(1); expect(platform.ensureIssueClosing.mock.calls[0][0]).toBe( config.dependencyDashboardTitle, @@ -354,7 +381,12 @@ describe('workers/repository/dependency-dashboard', () => { ]; config.dependencyDashboard = true; config.dependencyDashboardAutoclose = true; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(1); expect(platform.ensureIssueClosing.mock.calls[0][0]).toBe( config.dependencyDashboardTitle, @@ -370,7 +402,12 @@ describe('workers/repository/dependency-dashboard', () => { config.dependencyDashboard = true; config.dependencyDashboardHeader = 'This is a header'; config.dependencyDashboardFooter = 'And this is a footer'; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].title).toBe( @@ -395,7 +432,12 @@ describe('workers/repository/dependency-dashboard', () => { 'This is a header for platform:{{platform}}'; config.dependencyDashboardFooter = 'And this is a footer for repository:{{repository}}'; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].title).toBe( @@ -481,7 +523,12 @@ describe('workers/repository/dependency-dashboard', () => { }, ]; config.dependencyDashboard = true; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].title).toBe( @@ -518,7 +565,12 @@ describe('workers/repository/dependency-dashboard', () => { }, ]; config.dependencyDashboard = true; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].title).toBe( @@ -563,7 +615,12 @@ describe('workers/repository/dependency-dashboard', () => { }, ]; config.dependencyDashboard = true; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].title).toBe( @@ -598,7 +655,12 @@ describe('workers/repository/dependency-dashboard', () => { }, ]; config.dependencyDashboard = true; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].title).toBe( @@ -648,7 +710,12 @@ describe('workers/repository/dependency-dashboard', () => { ]; config.dependencyDashboard = true; config.dependencyDashboardPrApproval = true; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].title).toBe( @@ -770,7 +837,12 @@ describe('workers/repository/dependency-dashboard', () => { 'This is a header for platform:{{platform}}'; config.dependencyDashboardFooter = 'And this is a footer for repository:{{repository}}'; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssueClosing).toHaveBeenCalledTimes(0); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].title).toBe( @@ -825,7 +897,12 @@ describe('workers/repository/dependency-dashboard', () => { }, ]); config.dependencyDashboard = true; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); }); @@ -853,7 +930,12 @@ describe('workers/repository/dependency-dashboard', () => { repoProblemsHeader: 'platform is {{platform}}', }; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toContain( @@ -897,7 +979,12 @@ describe('workers/repository/dependency-dashboard', () => { - [ ] pr2 - [x] 🔐 **Create all pending approval PRs at once** 🔐`, }); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); const checkApprovePendingSelectAll = regEx( / - \[ ] /g, ); @@ -956,7 +1043,12 @@ describe('workers/repository/dependency-dashboard', () => { - [ ] pr1 - [ ] pr2`, }); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); const checkRateLimitedSelectAll = regEx( / - \[ ] /g, ); @@ -1034,7 +1126,12 @@ describe('workers/repository/dependency-dashboard', () => { - [x] ' `, }); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); }); @@ -1059,7 +1156,12 @@ None detected title: 'Dependency Dashboard', body: '', }); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).not.toHaveBeenCalled(); }); @@ -1067,7 +1169,12 @@ None detected const branches: BranchConfig[] = []; config.dependencyDashboard = true; config.dependencyDashboardLabels = ['RenovateBot', 'Maintenance']; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].labels).toStrictEqual([ 'RenovateBot', @@ -1099,7 +1206,12 @@ None detected it('add detected dependencies to the Dependency Dashboard body', async () => { const branches: BranchConfig[] = []; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); @@ -1111,7 +1223,12 @@ None detected const branches: BranchConfig[] = []; PackageFiles.clear(); PackageFiles.add('main', {}); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); @@ -1123,7 +1240,12 @@ None detected const branches: BranchConfig[] = []; PackageFiles.clear(); PackageFiles.add('main', null); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); @@ -1134,7 +1256,12 @@ None detected it('shows different combinations of version+digest for a given dependency', async () => { const branches: BranchConfig[] = []; PackageFiles.add('main', packageFilesWithDigest); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); @@ -1158,6 +1285,7 @@ None detected config, branches, packageFilesWithDeprecations, + { result: 'no-migration' }, ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toInclude( @@ -1183,7 +1311,12 @@ None detected it('add detected dependencies to the Dependency Dashboard body', async () => { const branches: BranchConfig[] = []; - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); @@ -1194,7 +1327,12 @@ None detected it('show default message in issues body when packageFiles is empty', async () => { const branches: BranchConfig[] = []; PackageFiles.add('main', {}); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); @@ -1205,7 +1343,12 @@ None detected it('show default message in issues body when when packageFiles is null', async () => { const branches: BranchConfig[] = []; PackageFiles.add('main', null); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); @@ -1218,7 +1361,12 @@ None detected const packageFilesBigRepo = genRandPackageFile(100, 700); PackageFiles.clear(); PackageFiles.add('main', packageFilesBigRepo); - await dependencyDashboard.ensureDependencyDashboard(config, branches); + await dependencyDashboard.ensureDependencyDashboard( + config, + branches, + {}, + { result: 'no-migration' }, + ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect( platform.ensureIssue.mock.calls[0][0].body.length < @@ -1257,6 +1405,7 @@ None detected config, branches, packageFiles, + { result: 'no-migration' }, ); expect(platform.ensureIssue).toHaveBeenCalledTimes(1); expect(platform.ensureIssue.mock.calls[0][0].body).toMatchSnapshot(); diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 2dd2a881008116..b6ce34f524c7d4 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -111,8 +111,8 @@ function parseDashboardIssue(issueBody: string): DependencyDashboard { const dependencyDashboardAllPending = checkApproveAllPendingPR(issueBody); const dependencyDashboardAllRateLimited = checkOpenAllRateLimitedPR(issueBody); - dependencyDashboardChecks['configMigrationInfo'] = - checkCreateConfigMigrationPr(issueBody); + dependencyDashboardChecks['configMigrationCheckboxState'] = + getConfigMigrationCheckboxState(issueBody); return { dependencyDashboardChecks, dependencyDashboardRebaseAllOpen, From f0e56c43a66b8451879426ac88d65ddf25157cf4 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Tue, 24 Sep 2024 03:37:42 +0530 Subject: [PATCH 25/31] docs: apply suggestions --- docs/usage/config-migration.md | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index 333c31ec93f0e0..2636a175342367 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -1,20 +1,24 @@ # Config Migration -Renovate maintainers often need to rename, remove or combine configuration options to improve the user experience and mature the product. +To improve the Renovate program, the Renovate maintainers often rename, remove or combine configuration options. -When they do so, "config migration" code is added at the same time so that any "legacy" config from users continues to work. -Config migration works by migrating legacy config internally before the config is used. -If done right, it "just works" silently and legacy configs continue working indefinitely. -The only sign when this is necessary is a debug message in logs noting the old and newly migrated configs. +When the Renovate maintainers change configuration options, they add "config migration" code. +The migration code allows "legacy" config from users to keep working. +Config migration works by migrating legacy config internally, before the config is used. +If done right, config migration "just works" silently and legacy configs continue working indefinitely. +The only sign that "config migration" is needed is the debug message in the Renovate logs, noting the old and newly migrated configs. By default, none of these changes are applied to Renovate config files (e.g. `renovate.json`) ## Enabling config migration pull requests -Although legacy config should continue to "just work", it's better for users if their config file uses the latest/correct configuration names. -Using the latest names makes it easier to understand the config and look up documentation for it. +Even though Renovate allows you to keep using "legacy config", we recommend you update the configuration names in your config regularly. +Using the latest names: -Renovate can create a config migration pull request, that migrates legacy config in your configuration file. +- makes it easier for you to understand the config +- helps you find the documentation for the config + +Renovate can create a config migration Pull Request, to migrate legacy config in your configuration file. To get config migration pull requests from Renovate: set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. Config migration PRs are disabled by default. @@ -46,7 +50,7 @@ Renovate will: ### Config migration needed, but disabled -If config migration is needed, but disabled then Renovate adds a checkbox to the Dependency Dashboard if one exists. +If config migration is needed, but disabled then Renovate adds a checkbox to the Dependency Dashboard if it exists. This is known as "on-demand" config migration because migration PRs are only created at the request of the user by ticking the checkbox. The checkbox looks like this: @@ -57,8 +61,8 @@ The checkbox looks like this: When you select the checkbox: -Renovate creates a config migration PR. -Renovate replaces the checkbox with a link to the Config Migration PR. +1. Renovate creates a config migration PR +2. Renovate replaces the checkbox with a link to the Config Migration PR For example: From 8af52349cbc82195500f1f32f0cbffaccbe20ec6 Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Thu, 26 Sep 2024 03:13:26 +0530 Subject: [PATCH 26/31] Update docs/usage/config-migration.md Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com> --- docs/usage/config-migration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index 2636a175342367..8a2b27d2a92817 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -50,7 +50,7 @@ Renovate will: ### Config migration needed, but disabled -If config migration is needed, but disabled then Renovate adds a checkbox to the Dependency Dashboard if it exists. +If config migration is needed, but disabled then Renovate adds a checkbox to the Dependency Dashboard (if the dashboard exists). This is known as "on-demand" config migration because migration PRs are only created at the request of the user by ticking the checkbox. The checkbox looks like this: From 084513fc849bc536ca8cf0578a4fa874bafc6f19 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Mon, 30 Sep 2024 18:40:16 +0530 Subject: [PATCH 27/31] apply suggestions --- lib/workers/repository/dependency-dashboard.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index b6ce34f524c7d4..0fe44b0240320e 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -292,7 +292,7 @@ export async function ensureDependencyDashboard( `See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; } else if (configMigrationRes?.result === 'pr-modified') { issueBody += - '## Config Migration Needed\n\n' + + '## Config Migration Needed (error)\n\n' + `The Config Migration branch exists but has been modified by another user. Renovate will not push to this branch unless it is first deleted. \n\n See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; } else if (configMigrationRes?.result === 'add-checkbox') { issueBody += From 9cf67caf9e6c90bc0320d07c1ebb475881edfb37 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Wed, 9 Oct 2024 14:30:15 +0200 Subject: [PATCH 28/31] Apply suggestions from code review --- docs/usage/config-migration.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index 8a2b27d2a92817..a77f1e7bad91cd 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -1,6 +1,6 @@ # Config Migration -To improve the Renovate program, the Renovate maintainers often rename, remove or combine configuration options. +As part of continuous improvement and refinement, the Renovate maintainers often rename, remove or combine configuration options. When the Renovate maintainers change configuration options, they add "config migration" code. The migration code allows "legacy" config from users to keep working. @@ -8,8 +8,6 @@ Config migration works by migrating legacy config internally, before the config If done right, config migration "just works" silently and legacy configs continue working indefinitely. The only sign that "config migration" is needed is the debug message in the Renovate logs, noting the old and newly migrated configs. -By default, none of these changes are applied to Renovate config files (e.g. `renovate.json`) - ## Enabling config migration pull requests Even though Renovate allows you to keep using "legacy config", we recommend you update the configuration names in your config regularly. @@ -19,10 +17,10 @@ Using the latest names: - helps you find the documentation for the config Renovate can create a config migration Pull Request, to migrate legacy config in your configuration file. -To get config migration pull requests from Renovate: set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. +To get automated config migration pull requests from Renovate: set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. Config migration PRs are disabled by default. -But we _strongly_ recommend you enable config migration PRs, because: +But we recommend you enable config migration PRs, because: - the config migration PR "tells" you something changed - up-to-date terms help you search the Renovate documentation From d2b3427b8304c1213629d5b25e70cbbbf49cbf8b Mon Sep 17 00:00:00 2001 From: RahulGautamSingh Date: Wed, 9 Oct 2024 21:38:43 +0530 Subject: [PATCH 29/31] Apply suggestions from code review Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com> --- docs/usage/config-migration.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/usage/config-migration.md b/docs/usage/config-migration.md index a77f1e7bad91cd..789e8ad5950d71 100644 --- a/docs/usage/config-migration.md +++ b/docs/usage/config-migration.md @@ -17,7 +17,7 @@ Using the latest names: - helps you find the documentation for the config Renovate can create a config migration Pull Request, to migrate legacy config in your configuration file. -To get automated config migration pull requests from Renovate: set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. +To get automated config migration Pull Requests from Renovate: set the [`configMigration`](./configuration-options.md#configmigration) config option to `true`. Config migration PRs are disabled by default. But we recommend you enable config migration PRs, because: @@ -44,12 +44,12 @@ Renovate takes no action. Renovate will: 1. Create a Config Migration PR -1. If the Dependency Dashboard issue is enabled then Renovate puts a link to the Config Migration PR on the dashboard +1. If the Dependency Dashboard issue is enabled, then Renovate puts a link to the Config Migration PR on the dashboard ### Config migration needed, but disabled If config migration is needed, but disabled then Renovate adds a checkbox to the Dependency Dashboard (if the dashboard exists). -This is known as "on-demand" config migration because migration PRs are only created at the request of the user by ticking the checkbox. +This is known as "on-demand" config migration, because migration PRs are only created at the request of the user by ticking the checkbox. The checkbox looks like this: From 7ac39a33ff7817414d527e74fc41be29a4a1f424 Mon Sep 17 00:00:00 2001 From: Rhys Arkins Date: Fri, 11 Oct 2024 10:38:03 +0200 Subject: [PATCH 30/31] Update lib/workers/repository/config-migration/index.ts Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com> --- lib/workers/repository/config-migration/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/workers/repository/config-migration/index.ts b/lib/workers/repository/config-migration/index.ts index 2da923acc33830..2574fdc4eacefb 100644 --- a/lib/workers/repository/config-migration/index.ts +++ b/lib/workers/repository/config-migration/index.ts @@ -40,7 +40,7 @@ export async function configMigration( const pr = await ensureConfigMigrationPr(config, migratedConfigData); // only happens incase a migration pr was created by another user - // for other cases in which a pr could not be found/created we log warning and throw error from within the ensureConfigMigrationPr fn + // for other cases in which a PR could not be found or created: we log warning and throw error from within the ensureConfigMigrationPr fn if (!pr) { MigratedDataFactory.reset(); return { result: 'add-checkbox' }; From 6ebbfe91e9b4e4691d0dd6658a0544d8248eb5df Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Tue, 15 Oct 2024 23:00:33 +0530 Subject: [PATCH 31/31] use comment as anchor --- lib/workers/repository/dependency-dashboard.spec.ts | 8 ++++---- lib/workers/repository/dependency-dashboard.ts | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/workers/repository/dependency-dashboard.spec.ts b/lib/workers/repository/dependency-dashboard.spec.ts index d35fccf21d8bef..4a67016793c32e 100644 --- a/lib/workers/repository/dependency-dashboard.spec.ts +++ b/lib/workers/repository/dependency-dashboard.spec.ts @@ -227,7 +227,7 @@ describe('workers/repository/dependency-dashboard', () => { platform.findIssue.mockResolvedValueOnce({ title: '', number: 1, - body: '\n\n - [x] Config Migration needed.', + body: '\n\n - [x] ', }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ @@ -241,7 +241,7 @@ describe('workers/repository/dependency-dashboard', () => { platform.findIssue.mockResolvedValueOnce({ title: '', number: 1, - body: '\n\n - [ ] Config Migration needed.', + body: '\n\n - [ ] ', }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ @@ -255,7 +255,7 @@ describe('workers/repository/dependency-dashboard', () => { platform.findIssue.mockResolvedValueOnce({ title: '', number: 1, - body: '\n\n Config Migration needed. See Config Migration PR: #3.', + body: '\n\n ', }); await dependencyDashboard.readDashboardBody(conf); expect(conf.dependencyDashboardChecks).toEqual({ @@ -788,7 +788,7 @@ describe('workers/repository/dependency-dashboard', () => { config.dependencyDashboardTitle, ); expect(platform.ensureIssue.mock.calls[0][0].body).toMatch( - '## Config Migration Needed\n\nSee Config Migration PR:', + `## Config Migration Needed\n\n See Config Migration PR:`, ); }); diff --git a/lib/workers/repository/dependency-dashboard.ts b/lib/workers/repository/dependency-dashboard.ts index 0fe44b0240320e..9c61879a877aa9 100644 --- a/lib/workers/repository/dependency-dashboard.ts +++ b/lib/workers/repository/dependency-dashboard.ts @@ -51,8 +51,8 @@ function checkRebaseAll(issueBody: string): boolean { function getConfigMigrationCheckboxState( issueBody: string, ): 'no-checkbox' | 'checked' | 'unchecked' | 'migration-pr-exists' { - if (!issueBody.includes('Config Migration needed.')) { - return 'no-checkbox'; + if (issueBody.includes('')) { + return 'migration-pr-exists'; } if (issueBody.includes(' - [x] ')) { @@ -63,7 +63,7 @@ function getConfigMigrationCheckboxState( return 'unchecked'; } - return 'migration-pr-exists'; + return 'no-checkbox'; } function selectAllRelevantBranches(issueBody: string): string[] { @@ -289,11 +289,11 @@ export async function ensureDependencyDashboard( if (configMigrationRes.result === 'pr-exists') { issueBody += '## Config Migration Needed\n\n' + - `See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; + ` See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; } else if (configMigrationRes?.result === 'pr-modified') { issueBody += '## Config Migration Needed (error)\n\n' + - `The Config Migration branch exists but has been modified by another user. Renovate will not push to this branch unless it is first deleted. \n\n See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; + ` The Config Migration branch exists but has been modified by another user. Renovate will not push to this branch unless it is first deleted. \n\n See Config Migration PR: #${configMigrationRes.prNumber}.\n\n`; } else if (configMigrationRes?.result === 'add-checkbox') { issueBody += '## Config Migration Needed\n\n' +