From 576d4d897a0f4186db7c92d7ff137f22aceda36a Mon Sep 17 00:00:00 2001 From: Gabriel-Ladzaretti <97394622+Gabriel-Ladzaretti@users.noreply.github.com> Date: Wed, 16 Nov 2022 07:04:39 +0200 Subject: [PATCH] refactor(config-migration): invoke `applyPrettierFormatting` at the commit stage (#18150) --- .../branch/__fixtures__/migrated-data.json | 7 +- .../branch/__fixtures__/migrated-data.json5 | 7 +- .../config-migration/branch/create.spec.ts | 20 +++- .../config-migration/branch/create.ts | 5 +- .../branch/migrated-data.spec.ts | 102 +++++++++++------- .../config-migration/branch/migrated-data.ts | 84 +++++++++------ .../config-migration/branch/rebase.spec.ts | 41 ++++++- .../config-migration/branch/rebase.ts | 29 ++++- .../repository/config-migration/index.spec.ts | 4 +- .../config-migration/pr/index.spec.ts | 3 + 10 files changed, 220 insertions(+), 82 deletions(-) diff --git a/lib/workers/repository/config-migration/branch/__fixtures__/migrated-data.json b/lib/workers/repository/config-migration/branch/__fixtures__/migrated-data.json index 9febe12a31cb7f..440ee5392db4da 100644 --- a/lib/workers/repository/config-migration/branch/__fixtures__/migrated-data.json +++ b/lib/workers/repository/config-migration/branch/__fixtures__/migrated-data.json @@ -1,4 +1,9 @@ { + "content": "{\n \"extends\": [\n \":separateMajorReleases\",\n \":prImmediately\",\n \":renovatePrefix\",\n \":semanticPrefixFixDepsChoreOthers\",\n \":updateNotScheduled\",\n \":automergeDisabled\",\n \":maintainLockFilesDisabled\",\n \":autodetectPinVersions\",\n \"group:monorepos\"\n ],\n \"onboarding\": false,\n \"rangeStrategy\": \"replace\",\n \"semanticCommits\": \"enabled\",\n \"timezone\": \"US/Central\",\n \"baseBranches\": [\n \"main\"\n ]\n}\n", "filename": "renovate.json", - "content": "{\n \"extends\": [\n \":separateMajorReleases\",\n \":prImmediately\",\n \":renovatePrefix\",\n \":semanticPrefixFixDepsChoreOthers\",\n \":updateNotScheduled\",\n \":automergeDisabled\",\n \":maintainLockFilesDisabled\",\n \":autodetectPinVersions\",\n \"group:monorepos\"\n ],\n \"onboarding\": false,\n \"rangeStrategy\": \"replace\",\n \"semanticCommits\": \"enabled\",\n \"timezone\": \"US/Central\",\n \"baseBranches\": [\n \"main\"\n ]\n}\n" + "indent": { + "amount": 2, + "indent": " ", + "type": "space" + } } diff --git a/lib/workers/repository/config-migration/branch/__fixtures__/migrated-data.json5 b/lib/workers/repository/config-migration/branch/__fixtures__/migrated-data.json5 index 0540d1f8ccb642..259972dafa88f5 100644 --- a/lib/workers/repository/config-migration/branch/__fixtures__/migrated-data.json5 +++ b/lib/workers/repository/config-migration/branch/__fixtures__/migrated-data.json5 @@ -1,4 +1,9 @@ { + "content": "{\n extends: [\n ':separateMajorReleases',\n ':prImmediately',\n ':renovatePrefix',\n ':semanticPrefixFixDepsChoreOthers',\n ':updateNotScheduled',\n ':automergeDisabled',\n ':maintainLockFilesDisabled',\n ':autodetectPinVersions',\n 'group:monorepos',\n ],\n onboarding: false,\n rangeStrategy: 'replace',\n semanticCommits: 'enabled',\n timezone: 'US/Central',\n baseBranches: [\n 'main',\n ],\n}\n", "filename": "renovate.json5", - "content": "{\n extends: [\n ':separateMajorReleases',\n ':prImmediately',\n ':renovatePrefix',\n ':semanticPrefixFixDepsChoreOthers',\n ':updateNotScheduled',\n ':automergeDisabled',\n ':maintainLockFilesDisabled',\n ':autodetectPinVersions',\n 'group:monorepos',\n ],\n onboarding: false,\n rangeStrategy: 'replace',\n semanticCommits: 'enabled',\n timezone: 'US/Central',\n baseBranches: [\n 'main',\n ],\n}\n" + "indent": { + "amount": 2, + "indent": " ", + "type": "space" + } } diff --git a/lib/workers/repository/config-migration/branch/create.spec.ts b/lib/workers/repository/config-migration/branch/create.spec.ts index e775ea6579af5a..90896f8582dd2c 100644 --- a/lib/workers/repository/config-migration/branch/create.spec.ts +++ b/lib/workers/repository/config-migration/branch/create.spec.ts @@ -1,7 +1,14 @@ +import type { Indent } from 'detect-indent'; import { Fixtures } from '../../../../../test/fixtures'; -import { RenovateConfig, getConfig, platform } from '../../../../../test/util'; +import { + RenovateConfig, + getConfig, + partial, + platform, +} from '../../../../../test/util'; import { checkoutBranch, commitFiles } from '../../../../util/git'; import { createConfigMigrationBranch } from './create'; +import { MigratedDataFactory } from './migrated-data'; import type { MigratedData } from './migrated-data'; jest.mock('../../../../util/git'); @@ -11,6 +18,10 @@ describe('workers/repository/config-migration/branch/create', () => { const indent = ' '; const renovateConfig = JSON.stringify(raw, undefined, indent) + '\n'; const filename = 'renovate.json'; + const prettierSpy = jest.spyOn( + MigratedDataFactory, + 'applyPrettierFormatting' + ); let config: RenovateConfig; let migratedConfigData: MigratedData; @@ -19,7 +30,12 @@ describe('workers/repository/config-migration/branch/create', () => { config = getConfig(); config.baseBranch = 'dev'; config.defaultBranch = 'master'; - migratedConfigData = { content: renovateConfig, filename }; + migratedConfigData = { + content: renovateConfig, + filename, + indent: partial({}), + }; + prettierSpy.mockResolvedValueOnce(migratedConfigData.content); }); describe('createConfigMigrationBranch', () => { diff --git a/lib/workers/repository/config-migration/branch/create.ts b/lib/workers/repository/config-migration/branch/create.ts index 0e8bbe74dccfb1..cd337ad37bfa6d 100644 --- a/lib/workers/repository/config-migration/branch/create.ts +++ b/lib/workers/repository/config-migration/branch/create.ts @@ -5,6 +5,7 @@ import { commitAndPush } from '../../../../modules/platform/commit'; import { checkoutBranch } from '../../../../util/git'; import { getMigrationBranchName } from '../common'; import { ConfigMigrationCommitMessageFactory } from './commit-message'; +import { MigratedDataFactory } from './migrated-data'; import type { MigratedData } from './migrated-data'; export async function createConfigMigrationBranch( @@ -12,7 +13,6 @@ export async function createConfigMigrationBranch( migratedConfigData: MigratedData ): Promise { logger.debug('createConfigMigrationBranch()'); - const contents = migratedConfigData.content; const configFileName = migratedConfigData.filename; logger.debug('Creating config migration branch'); @@ -30,6 +30,9 @@ export async function createConfigMigrationBranch( } await checkoutBranch(config.defaultBranch!); + const contents = await MigratedDataFactory.applyPrettierFormatting( + migratedConfigData + ); return commitAndPush({ branchName: getMigrationBranchName(config), files: [ diff --git a/lib/workers/repository/config-migration/branch/migrated-data.spec.ts b/lib/workers/repository/config-migration/branch/migrated-data.spec.ts index b43ca2b6f801e9..09f7bcecc13af8 100644 --- a/lib/workers/repository/config-migration/branch/migrated-data.spec.ts +++ b/lib/workers/repository/config-migration/branch/migrated-data.spec.ts @@ -7,7 +7,7 @@ import { logger } from '../../../../logger'; import { readLocalFile } from '../../../../util/fs'; import { getFileList } from '../../../../util/git'; import { detectRepoFileConfig } from '../../init/merge'; -import { MigratedDataFactory, applyPrettierFormatting } from './migrated-data'; +import { MigratedDataFactory } from './migrated-data'; jest.mock('../../../../config/migration'); jest.mock('../../../../util/git'); @@ -41,7 +41,6 @@ describe('workers/repository/config-migration/branch/migrated-data', () => { isMigrated: true, migratedConfig: migratedConfigObj, }); - mockedFunction(getFileList).mockResolvedValue([]); }); it('Calls getAsync a first when migration not needed', async () => { @@ -86,16 +85,18 @@ describe('workers/repository/config-migration/branch/migrated-data', () => { }); it('Resets the factory and gets a new value with default indentation', async () => { - mockedFunction(detectIndent).mockReturnValueOnce({ + const indent = { type: undefined, amount: 0, // TODO: incompatible types (#7154) indent: null as never, - }); + }; + mockedFunction(detectIndent).mockReturnValueOnce(indent); MigratedDataFactory.reset(); - await expect(MigratedDataFactory.getAsync()).resolves.toEqual( - migratedData - ); + await expect(MigratedDataFactory.getAsync()).resolves.toEqual({ + ...migratedData, + indent, + }); }); it('Migrate a JSON5 config file', async () => { @@ -119,49 +120,76 @@ describe('workers/repository/config-migration/branch/migrated-data', () => { 'MigratedDataFactory.getAsync() Error initializing renovate MigratedData' ); }); + }); - it('format and migrate a JSON config file', async () => { + describe('MigratedDataFactory.applyPrettierFormatting', () => { + beforeAll(() => { + mockedFunction(detectIndent).mockReturnValueOnce({ + type: 'space', + amount: 2, + indent: ' ', + }); mockedFunction(detectRepoFileConfig).mockResolvedValueOnce({ configFileName: 'renovate.json', + configFileRaw: rawNonMigrated, + }); + mockedFunction(migrateConfig).mockReturnValueOnce({ + isMigrated: true, + migratedConfig: migratedConfigObj, }); - mockedFunction(getFileList).mockResolvedValue(['.prettierrc']); MigratedDataFactory.reset(); - await expect(MigratedDataFactory.getAsync()).resolves.toEqual( - formattedMigratedData - ); }); - it('should not stop run for invalid package.json', async () => { - mockedFunction(detectRepoFileConfig).mockResolvedValueOnce({ - configFileName: 'renovate.json', - configFileRaw: 'abci', - }); - mockedFunction(readLocalFile).mockResolvedValueOnce(rawNonMigrated); - MigratedDataFactory.reset(); - await expect(MigratedDataFactory.getAsync()).resolves.toEqual( - migratedData - ); + beforeEach(() => { + mockedFunction(getFileList).mockResolvedValue([]); }); - it('should not stop run for readLocalFile error', async () => { - mockedFunction(detectRepoFileConfig).mockResolvedValueOnce({ - configFileName: 'renovate.json', - configFileRaw: 'abci', - }); + it('does not format when no prettier config is present', async () => { + const { content: unformatted } = migratedData; + mockedFunction(readLocalFile).mockResolvedValueOnce(null); + await MigratedDataFactory.getAsync(); + await expect( + MigratedDataFactory.applyPrettierFormatting(migratedData) + ).resolves.toEqual(unformatted); + }); + + it('does not format when failing to fetch package.json file', async () => { + const { content: unformatted } = migratedData; mockedFunction(readLocalFile).mockRejectedValueOnce(null); - MigratedDataFactory.reset(); - await expect(MigratedDataFactory.getAsync()).resolves.toEqual( - migratedData - ); + await MigratedDataFactory.getAsync(); + await expect( + MigratedDataFactory.applyPrettierFormatting(migratedData) + ).resolves.toEqual(unformatted); + }); + + it('does not format when there is an invalid package.json file', async () => { + const { content: unformatted } = migratedData; + mockedFunction(readLocalFile).mockResolvedValueOnce('invalid json'); + await MigratedDataFactory.getAsync(); + await expect( + MigratedDataFactory.applyPrettierFormatting(migratedData) + ).resolves.toEqual(unformatted); + }); + + it('formats when prettier config file is found', async () => { + const formatted = formattedMigratedData.content; + mockedFunction(getFileList).mockResolvedValue(['.prettierrc']); + await MigratedDataFactory.getAsync(); + await expect( + MigratedDataFactory.applyPrettierFormatting(migratedData) + ).resolves.toEqual(formatted); }); - it('return original content if its invalid', async () => { + it('formats when finds prettier config inside the package.json file', async () => { + const formatted = formattedMigratedData.content; + mockedFunction(detectRepoFileConfig).mockResolvedValueOnce({ + configFileName: 'renovate.json', + }); + mockedFunction(readLocalFile).mockResolvedValueOnce('{"prettier":{}}'); + await MigratedDataFactory.getAsync(); await expect( - applyPrettierFormatting(`{"name":"Rahul"`, 'json', { - indent: ' ', - amount: 2, - }) - ).resolves.toBe(`{"name":"Rahul"`); + MigratedDataFactory.applyPrettierFormatting(migratedData) + ).resolves.toEqual(formatted); }); }); }); diff --git a/lib/workers/repository/config-migration/branch/migrated-data.ts b/lib/workers/repository/config-migration/branch/migrated-data.ts index a6062a49a38f04..0a06ebca7f36ff 100644 --- a/lib/workers/repository/config-migration/branch/migrated-data.ts +++ b/lib/workers/repository/config-migration/branch/migrated-data.ts @@ -1,6 +1,7 @@ import detectIndent from 'detect-indent'; import JSON5 from 'json5'; -import prettier from 'prettier'; +import prettier, { BuiltInParserName } from 'prettier'; +import upath from 'upath'; import { migrateConfig } from '../../../../config/migration'; import { logger } from '../../../../logger'; import { readLocalFile } from '../../../../util/fs'; @@ -10,7 +11,9 @@ import { detectRepoFileConfig } from '../../init/merge'; export interface MigratedData { content: string; filename: string; + indent: Indent; } + interface Indent { amount: number; indent: string; @@ -30,44 +33,52 @@ const prettierConfigFilenames = new Set([ '.prettierrc.toml', ]); +export type PrettierParser = BuiltInParserName; + export async function applyPrettierFormatting( content: string, - parser: string, - indent: Indent + parser: PrettierParser, + indent?: Indent ): Promise { - const fileList = await getFileList(); - let prettierExists = fileList.some((file) => - prettierConfigFilenames.has(file) - ); - if (!prettierExists) { - try { - const packageJsonContent = await readLocalFile('package.json', 'utf8'); - prettierExists = - packageJsonContent && JSON.parse(packageJsonContent).prettier; - } catch { - logger.warn( - 'applyPrettierFormatting() - Error processing package.json file' - ); + try { + logger.trace('applyPrettierFormatting - START'); + const fileList = await getFileList(); + let prettierExists = fileList.some((file) => + prettierConfigFilenames.has(file) + ); + + if (!prettierExists) { + try { + const packageJsonContent = await readLocalFile('package.json', 'utf8'); + prettierExists = + packageJsonContent && JSON.parse(packageJsonContent).prettier; + } catch { + logger.warn( + 'applyPrettierFormatting - Error processing package.json file' + ); + } } - } - if (!prettierExists) { - return content; + if (!prettierExists) { + return content; + } + const options = { + parser, + tabWidth: indent?.amount === 0 ? 2 : indent?.amount, + useTabs: indent?.type === 'tab', + }; + + return prettier.format(content, options); + } finally { + logger.trace('applyPrettierFormatting - END'); } - const options = { - parser, - tabWidth: indent.amount === 0 ? 2 : indent.amount, - useTabs: indent.type === 'tab', - }; - - return prettier.format(content, options); } export class MigratedDataFactory { // singleton private static data: MigratedData | null; - public static async getAsync(): Promise { + static async getAsync(): Promise { if (this.data) { return this.data; } @@ -81,10 +92,19 @@ export class MigratedDataFactory { return this.data; } - public static reset(): void { + static reset(): void { this.data = null; } + static applyPrettierFormatting({ + content, + filename, + indent, + }: MigratedData): Promise { + const parser = upath.extname(filename).replace('.', '') as PrettierParser; + return applyPrettierFormatting(content, parser, indent); + } + private static async build(): Promise { let res: MigratedData | null = null; try { @@ -116,17 +136,11 @@ export class MigratedDataFactory { content = JSON.stringify(migratedConfig, undefined, indentSpace); } - // format if prettier is found in the user's repo - content = await applyPrettierFormatting( - content, - filename.endsWith('.json5') ? 'json5' : 'json', - indent - ); if (!content.endsWith('\n')) { content += '\n'; } - res = { content, filename }; + res = { content, filename, indent }; } catch (err) { logger.debug( { err }, diff --git a/lib/workers/repository/config-migration/branch/rebase.spec.ts b/lib/workers/repository/config-migration/branch/rebase.spec.ts index 81ab12b7f5e494..a9f647c99845bf 100644 --- a/lib/workers/repository/config-migration/branch/rebase.spec.ts +++ b/lib/workers/repository/config-migration/branch/rebase.spec.ts @@ -1,18 +1,30 @@ +import type { Indent } from 'detect-indent'; import { Fixtures } from '../../../../../test/fixtures'; import { RenovateConfig, getConfig, git, + partial, platform, } from '../../../../../test/util'; import { GlobalConfig } from '../../../../config/global'; -import { checkoutBranch } from '../../../../util/git'; +import { checkoutBranch, commitFiles } from '../../../../util/git'; +import { MigratedDataFactory } from './migrated-data'; import type { MigratedData } from './migrated-data'; import { rebaseMigrationBranch } from './rebase'; jest.mock('../../../../util/git'); +const formattedMigratedData = Fixtures.getJson( + './migrated-data-formatted.json' +); + describe('workers/repository/config-migration/branch/rebase', () => { + const prettierSpy = jest.spyOn( + MigratedDataFactory, + 'applyPrettierFormatting' + ); + beforeAll(() => { GlobalConfig.set({ localDir: '', @@ -31,7 +43,11 @@ describe('workers/repository/config-migration/branch/rebase', () => { beforeEach(() => { jest.resetAllMocks(); GlobalConfig.reset(); - migratedConfigData = { content: renovateConfig, filename }; + migratedConfigData = { + content: renovateConfig, + filename, + indent: partial({}), + }; config = { ...getConfig(), repository: 'some/repo', @@ -63,6 +79,27 @@ describe('workers/repository/config-migration/branch/rebase', () => { expect(git.commitFiles).toHaveBeenCalledTimes(1); }); + it('applies prettier formatting when rebasing the migration branch ', async () => { + const formatted = formattedMigratedData.content; + prettierSpy.mockResolvedValueOnce(formattedMigratedData.content); + git.isBranchBehindBase.mockResolvedValueOnce(true); + await rebaseMigrationBranch(config, migratedConfigData); + expect(checkoutBranch).toHaveBeenCalledWith(config.defaultBranch); + expect(git.commitFiles).toHaveBeenCalledTimes(1); + expect(commitFiles).toHaveBeenCalledWith({ + branchName: 'renovate/migrate-config', + files: [ + { + type: 'addition', + path: 'renovate.json', + contents: formatted, + }, + ], + message: 'Migrate config renovate.json', + platformCommit: false, + }); + }); + it('does not rebases migration branch when in dryRun is on', async () => { GlobalConfig.set({ dryRun: 'full', diff --git a/lib/workers/repository/config-migration/branch/rebase.ts b/lib/workers/repository/config-migration/branch/rebase.ts index 06a3800915a6f8..7a6a700ca8234a 100644 --- a/lib/workers/repository/config-migration/branch/rebase.ts +++ b/lib/workers/repository/config-migration/branch/rebase.ts @@ -7,8 +7,10 @@ import { getFile, isBranchModified, } from '../../../../util/git'; +import { quickStringify } from '../../../../util/stringify'; import { getMigrationBranchName } from '../common'; import { ConfigMigrationCommitMessageFactory } from './commit-message'; +import { MigratedDataFactory } from './migrated-data'; import type { MigratedData } from './migrated-data'; export async function rebaseMigrationBranch( @@ -22,9 +24,11 @@ export async function rebaseMigrationBranch( return null; } const configFileName = migratedConfigData.filename; - const contents = migratedConfigData.content; + let contents = migratedConfigData.content; const existingContents = await getFile(configFileName, branchName); - if (contents === existingContents) { + if ( + jsonStripWhitespaces(contents) === jsonStripWhitespaces(existingContents) + ) { logger.debug('Migration branch is up to date'); return null; } @@ -42,6 +46,9 @@ export async function rebaseMigrationBranch( const commitMessage = commitMessageFactory.getCommitMessage(); await checkoutBranch(config.defaultBranch!); + contents = await MigratedDataFactory.applyPrettierFormatting( + migratedConfigData + ); return commitAndPush({ branchName, files: [ @@ -55,3 +62,21 @@ export async function rebaseMigrationBranch( platformCommit: !!config.platformCommit, }); } + +/** + * @param json a JSON string + * @return a minimal json string. i.e. does not contain any formatting/whitespaces + */ +function jsonStripWhitespaces(json: string | null): string | null { + if (!json) { + return null; + } + /** + * JSON.stringify(value, replacer, space): + * If "space" is anything other than a string or number — + * for example, is null or not provided — no white space is used. + * + * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#parameters + */ + return quickStringify(JSON.parse(json)); +} diff --git a/lib/workers/repository/config-migration/index.spec.ts b/lib/workers/repository/config-migration/index.spec.ts index ad29c89f68e57a..f15f4bd75d5d75 100644 --- a/lib/workers/repository/config-migration/index.spec.ts +++ b/lib/workers/repository/config-migration/index.spec.ts @@ -1,5 +1,6 @@ +import type { Indent } from 'detect-indent'; import { Fixtures } from '../../../../test/fixtures'; -import { getConfig, mockedFunction } from '../../../../test/util'; +import { getConfig, mockedFunction, partial } from '../../../../test/util'; import { checkConfigMigrationBranch } from './branch'; import { MigratedDataFactory } from './branch/migrated-data'; import { ensureConfigMigrationPr } from './pr'; @@ -23,6 +24,7 @@ describe('workers/repository/config-migration/index', () => { mockedFunction(MigratedDataFactory.getAsync).mockResolvedValue({ filename, content, + indent: partial({}), }); }); diff --git a/lib/workers/repository/config-migration/pr/index.spec.ts b/lib/workers/repository/config-migration/pr/index.spec.ts index 6b2053ae61ccb9..48eaacbcc2a9e5 100644 --- a/lib/workers/repository/config-migration/pr/index.spec.ts +++ b/lib/workers/repository/config-migration/pr/index.spec.ts @@ -1,3 +1,4 @@ +import type { Indent } from 'detect-indent'; import type { RequestError, Response } from 'got'; import { mock } from 'jest-mock-extended'; import { Fixtures } from '../../../../../test/fixtures'; @@ -30,6 +31,7 @@ describe('workers/repository/config-migration/pr/index', () => { const migratedData: MigratedData = { content: migratedContent, filename: configFileName, + indent: partial({}), }; let config: RenovateConfig; @@ -166,6 +168,7 @@ describe('workers/repository/config-migration/pr/index', () => { await ensureConfigMigrationPr(config, { content: migratedContent, filename: 'renovate.json5', + indent: partial({}), }); expect(platform.createPr).toHaveBeenCalledTimes(1); expect(platform.createPr.mock.calls[0][0].prBody).toMatchSnapshot();