From 6e15fdc7d4bc578ed4ffa052f8f5caa97d0c8506 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Sun, 29 Dec 2024 00:52:53 +0530 Subject: [PATCH 1/3] reconfigure: refactor code structure --- lib/workers/repository/finalize/index.ts | 4 +- lib/workers/repository/finalize/prune.ts | 2 +- .../repository/reconfigure/index.spec.ts | 32 +-- lib/workers/repository/reconfigure/index.ts | 218 ++++-------------- lib/workers/repository/reconfigure/utils.ts | 3 + .../repository/reconfigure/validate.ts | 184 +++++++++++++++ 6 files changed, 248 insertions(+), 195 deletions(-) create mode 100644 lib/workers/repository/reconfigure/utils.ts create mode 100644 lib/workers/repository/reconfigure/validate.ts diff --git a/lib/workers/repository/finalize/index.ts b/lib/workers/repository/finalize/index.ts index e530834c7e..8878201696 100644 --- a/lib/workers/repository/finalize/index.ts +++ b/lib/workers/repository/finalize/index.ts @@ -4,7 +4,7 @@ import { platform } from '../../../modules/platform'; import * as repositoryCache from '../../../util/cache/repository'; import { clearRenovateRefs } from '../../../util/git'; import { PackageFiles } from '../package-files'; -import { validateReconfigureBranch } from '../reconfigure'; +import { checkReconfigureBranch } from '../reconfigure'; import { pruneStaleBranches } from './prune'; import { runBranchSummary, @@ -16,7 +16,7 @@ export async function finalizeRepo( config: RenovateConfig, branchList: string[], ): Promise { - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); await repositoryCache.saveCache(); await pruneStaleBranches(config, branchList); await ensureIssuesClosing(); diff --git a/lib/workers/repository/finalize/prune.ts b/lib/workers/repository/finalize/prune.ts index 7d30c01f97..918751344b 100644 --- a/lib/workers/repository/finalize/prune.ts +++ b/lib/workers/repository/finalize/prune.ts @@ -9,7 +9,7 @@ import { scm } from '../../../modules/platform/scm'; import { getBranchList, setUserRepoConfig } from '../../../util/git'; import { escapeRegExp, regEx } from '../../../util/regex'; import { uniqueStrings } from '../../../util/string'; -import { getReconfigureBranchName } from '../reconfigure'; +import { getReconfigureBranchName } from '../reconfigure/utils'; async function cleanUpBranches( config: RenovateConfig, diff --git a/lib/workers/repository/reconfigure/index.spec.ts b/lib/workers/repository/reconfigure/index.spec.ts index d7b9e5a97d..ccde73c93e 100644 --- a/lib/workers/repository/reconfigure/index.spec.ts +++ b/lib/workers/repository/reconfigure/index.spec.ts @@ -7,7 +7,7 @@ import type { Pr } from '../../../modules/platform/types'; import * as _cache from '../../../util/cache/repository'; import type { LongCommitSha } from '../../../util/git/types'; import * as _merge from '../init/merge'; -import { validateReconfigureBranch } from '.'; +import { checkReconfigureBranch } from '.'; jest.mock('../../../util/cache/repository'); jest.mock('../../../util/fs'); @@ -39,7 +39,7 @@ describe('workers/repository/reconfigure/index', () => { it('no effect when running with platform=local', async () => { GlobalConfig.set({ platform: 'local' }); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.debug).toHaveBeenCalledWith( 'Not attempting to reconfigure when running with local platform', ); @@ -47,14 +47,14 @@ describe('workers/repository/reconfigure/index', () => { it('no effect on repo with no reconfigure branch', async () => { scm.branchExists.mockResolvedValueOnce(false); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.debug).toHaveBeenCalledWith('No reconfigure branch found'); }); it('logs error if config file search fails', async () => { const err = new Error(); merge.detectConfigFile.mockRejectedValueOnce(err as never); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.error).toHaveBeenCalledWith( { err }, 'Error while searching for config file in reconfigure branch', @@ -63,7 +63,7 @@ describe('workers/repository/reconfigure/index', () => { it('throws error if config file not found in reconfigure branch', async () => { merge.detectConfigFile.mockResolvedValue(null); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.warn).toHaveBeenCalledWith( 'No config file found in reconfigure branch', ); @@ -72,7 +72,7 @@ describe('workers/repository/reconfigure/index', () => { it('logs error if config file is unreadable', async () => { const err = new Error(); fs.readLocalFile.mockRejectedValueOnce(err as never); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.error).toHaveBeenCalledWith( { err }, 'Error while reading config file', @@ -80,7 +80,7 @@ describe('workers/repository/reconfigure/index', () => { }); it('throws error if config file is empty', async () => { - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.warn).toHaveBeenCalledWith('Empty or invalid config file'); }); @@ -90,7 +90,7 @@ describe('workers/repository/reconfigure/index', () => { "name": } `); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.error).toHaveBeenCalledWith( { err: expect.any(Object) }, 'Error while parsing config file', @@ -109,7 +109,7 @@ describe('workers/repository/reconfigure/index', () => { "enabledManagers": ["docker"] } `); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.debug).toHaveBeenCalledWith( { errors: expect.any(String) }, 'Validation Errors', @@ -129,7 +129,7 @@ describe('workers/repository/reconfigure/index', () => { } `); platform.findPr.mockResolvedValueOnce(mock({ number: 1 })); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.debug).toHaveBeenCalledWith( { errors: expect.any(String) }, 'Validation Errors', @@ -148,7 +148,7 @@ describe('workers/repository/reconfigure/index', () => { `; merge.detectConfigFile.mockResolvedValue('package.json'); fs.readLocalFile.mockResolvedValueOnce(pJson).mockResolvedValueOnce(pJson); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(platform.setBranchStatus).toHaveBeenCalledWith({ branchName: 'prefix/reconfigure', context: 'renovate/config-validation', @@ -165,7 +165,7 @@ describe('workers/repository/reconfigure/index', () => { }, }); - await validateReconfigureBranch({ + await checkReconfigureBranch({ ...config, statusCheckNames: partial({ configValidation: null, @@ -185,7 +185,7 @@ describe('workers/repository/reconfigure/index', () => { }, }); - await validateReconfigureBranch({ + await checkReconfigureBranch({ ...config, statusCheckNames: partial({ configValidation: '', @@ -204,7 +204,7 @@ describe('workers/repository/reconfigure/index', () => { isConfigValid: false, }, }); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.debug).toHaveBeenCalledWith( 'Skipping validation check as branch sha is unchanged', ); @@ -218,7 +218,7 @@ describe('workers/repository/reconfigure/index', () => { }, }); platform.getBranchStatusCheck.mockResolvedValueOnce('green'); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(logger.debug).toHaveBeenCalledWith( 'Skipping validation check because status check already exists.', ); @@ -231,7 +231,7 @@ describe('workers/repository/reconfigure/index', () => { "enabledManagers": ["npm",] } `); - await validateReconfigureBranch(config); + await checkReconfigureBranch(config); expect(platform.setBranchStatus).toHaveBeenCalledWith({ branchName: 'prefix/reconfigure', context: 'renovate/config-validation', diff --git a/lib/workers/repository/reconfigure/index.ts b/lib/workers/repository/reconfigure/index.ts index abdb1d0146..8143bd3919 100644 --- a/lib/workers/repository/reconfigure/index.ts +++ b/lib/workers/repository/reconfigure/index.ts @@ -1,60 +1,37 @@ -import is from '@sindresorhus/is'; -import JSON5 from 'json5'; import { GlobalConfig } from '../../../config/global'; import type { RenovateConfig } from '../../../config/types'; -import { validateConfig } from '../../../config/validation'; import { logger } from '../../../logger'; import { platform } from '../../../modules/platform'; -import { ensureComment } from '../../../modules/platform/comment'; import { scm } from '../../../modules/platform/scm'; -import type { BranchStatus } from '../../../types'; -import { getCache } from '../../../util/cache/repository'; -import { readLocalFile } from '../../../util/fs'; -import { getBranchCommit } from '../../../util/git'; -import { regEx } from '../../../util/regex'; -import { detectConfigFile } from '../init/merge'; -import { - deleteReconfigureBranchCache, - setReconfigureBranchCache, -} from './reconfigure-cache'; +import { deleteReconfigureBranchCache } from './reconfigure-cache'; +import { getReconfigureBranchName } from './utils'; +import { validateReconfigureBranch } from './validate'; -async function setBranchStatus( - branchName: string, - description: string, - state: BranchStatus, - context?: string | null, -): Promise { - if (!is.nonEmptyString(context)) { - // already logged this case when validating the status check - return; - } +/** + * In a reconfigure branch: + * first check if such a branch exists (no->return) + * yes: + * check sha and only continue if sha doesn't match cached sha + * check if it has a valid config file (no failing check? and return) + * get the content, validate and give passing check as per it + * + * extractDeps + * ensure pr + */ - await platform.setBranchStatus({ - branchName, - context, - description, - state, - }); -} - -export function getReconfigureBranchName(prefix: string): string { - return `${prefix}reconfigure`; -} -export async function validateReconfigureBranch( +export async function checkReconfigureBranch( config: RenovateConfig, ): Promise { - logger.debug('validateReconfigureBranch()'); + logger.debug('checkReconfigureBranch()'); if (GlobalConfig.get('platform') === 'local') { logger.debug( - 'Not attempting to reconfigure when running with local platform', + 'Not attempting to check reconfigure branch when running with local platform', ); return; } - const context = config.statusCheckNames?.configValidation; - - const branchName = getReconfigureBranchName(config.branchPrefix!); - const branchExists = await scm.branchExists(branchName); + const reconfigureBranch = getReconfigureBranchName(config.branchPrefix!); + const branchExists = await scm.branchExists(reconfigureBranch); // this is something the user initiates, so skip if no branch exists if (!branchExists) { @@ -63,141 +40,30 @@ export async function validateReconfigureBranch( return; } - // look for config file - // 1. check reconfigure branch cache and use the configFileName if it exists - // 2. checkout reconfigure branch and look for the config file, don't assume default configFileName - const branchSha = getBranchCommit(branchName)!; - const cache = getCache(); - let configFileName: string | null = null; - const reconfigureCache = cache.reconfigureBranchCache; - // only use valid cached information - if (reconfigureCache?.reconfigureBranchSha === branchSha) { - logger.debug('Skipping validation check as branch sha is unchanged'); - return; + // check for the pr if it exists before hand so we do not need to make 2 calls + const reconfigurePr = await platform.findPr({ + branchName: reconfigureBranch, + state: 'open', + includeOtherAuthors: true, + }); + const { status } = await validateReconfigureBranch(config, reconfigurePr); + + // config is invalid someway + if (!status) { + logger.debug('Config is invalid skipping Pr creation'); } - if (context) { - const validationStatus = await platform.getBranchStatusCheck( - branchName, - context, - ); + // if status is true it mean 4 things + // 1. config file present + // 2. it has a valid config file name + // 3. it has valid json in its + // 4. the config in it is valid - // if old status check is present skip validation - if (is.nonEmptyString(validationStatus)) { - logger.debug( - 'Skipping validation check because status check already exists.', - ); - return; - } - } else { - logger.debug( - 'Status check is null or an empty string, skipping status check addition.', - ); - } - - try { - await scm.checkoutBranch(branchName); - configFileName = await detectConfigFile(); - } catch (err) { - logger.error( - { err }, - 'Error while searching for config file in reconfigure branch', - ); - } - - if (!is.nonEmptyString(configFileName)) { - logger.warn('No config file found in reconfigure branch'); - await setBranchStatus( - branchName, - 'Validation Failed - No config file found', - 'red', - context, - ); - setReconfigureBranchCache(branchSha, false); - await scm.checkoutBranch(config.defaultBranch!); - return; - } - - let configFileRaw: string | null = null; - try { - configFileRaw = await readLocalFile(configFileName, 'utf8'); - } catch (err) { - logger.error({ err }, 'Error while reading config file'); - } - - if (!is.nonEmptyString(configFileRaw)) { - logger.warn('Empty or invalid config file'); - await setBranchStatus( - branchName, - 'Validation Failed - Empty/Invalid config file', - 'red', - context, - ); - setReconfigureBranchCache(branchSha, false); - await scm.checkoutBranch(config.baseBranch!); - return; - } - - let configFileParsed: any; - try { - configFileParsed = JSON5.parse(configFileRaw); - // no need to confirm renovate field in package.json we already do it in `detectConfigFile()` - if (configFileName === 'package.json') { - configFileParsed = configFileParsed.renovate; - } - } catch (err) { - logger.error({ err }, 'Error while parsing config file'); - await setBranchStatus( - branchName, - 'Validation Failed - Unparsable config file', - 'red', - context, - ); - setReconfigureBranchCache(branchSha, false); - await scm.checkoutBranch(config.baseBranch!); - return; - } - - // perform validation and provide a passing or failing check run based on result - const validationResult = await validateConfig('repo', configFileParsed); - - // failing check - if (validationResult.errors.length > 0) { - logger.debug( - { errors: validationResult.errors.map((err) => err.message).join(', ') }, - 'Validation Errors', - ); - - // add comment to reconfigure PR if it exists - const branchPr = await platform.findPr({ - branchName, - state: 'open', - includeOtherAuthors: true, - }); - if (branchPr) { - let body = `There is an error with this repository's Renovate configuration that needs to be fixed.\n\n`; - body += `Location: \`${configFileName}\`\n`; - body += `Message: \`${validationResult.errors - .map((e) => e.message) - .join(', ') - .replace(regEx(/`/g), "'")}\`\n`; - - await ensureComment({ - number: branchPr.number, - topic: 'Action Required: Fix Renovate Configuration', - content: body, - }); - } - - await setBranchStatus(branchName, 'Validation Failed', 'red', context); - setReconfigureBranchCache(branchSha, false); - await scm.checkoutBranch(config.baseBranch!); - return; - } - - // passing check - await setBranchStatus(branchName, 'Validation Successful', 'green', context); - - setReconfigureBranchCache(branchSha, true); - await scm.checkoutBranch(config.baseBranch!); + // now we need to check if a pr is present or not + // and if it is then, see if we can update anything in it. + // first test how an onboarding pr looks like and the info in it + // can be done by test run a repo or we can check the code + // remaining query is whether: the vaidate reconfigure branch needs to be refactored further? + // how the cache will be handled + // what happens if sha is same? > nothing cause we do not lookup so no sha change do nothing: unless a or is missing do created that } diff --git a/lib/workers/repository/reconfigure/utils.ts b/lib/workers/repository/reconfigure/utils.ts new file mode 100644 index 0000000000..e5208d6a10 --- /dev/null +++ b/lib/workers/repository/reconfigure/utils.ts @@ -0,0 +1,3 @@ +export function getReconfigureBranchName(prefix: string): string { + return `${prefix}reconfigure`; +} diff --git a/lib/workers/repository/reconfigure/validate.ts b/lib/workers/repository/reconfigure/validate.ts new file mode 100644 index 0000000000..ca05398614 --- /dev/null +++ b/lib/workers/repository/reconfigure/validate.ts @@ -0,0 +1,184 @@ +import is from '@sindresorhus/is'; +import JSON5 from 'json5'; +import type { RenovateConfig } from '../../../config/types'; +import { validateConfig } from '../../../config/validation'; +import { logger } from '../../../logger'; +import type { Pr } from '../../../modules/platform'; +import { platform } from '../../../modules/platform'; +import { ensureComment } from '../../../modules/platform/comment'; +import { scm } from '../../../modules/platform/scm'; +import type { BranchStatus } from '../../../types'; +import { getCache } from '../../../util/cache/repository'; +import { readLocalFile } from '../../../util/fs'; +import { getBranchCommit } from '../../../util/git'; +import { regEx } from '../../../util/regex'; +import { detectConfigFile } from '../init/merge'; +import { setReconfigureBranchCache } from './reconfigure-cache'; +import { getReconfigureBranchName } from './utils'; + +async function setBranchStatus( + branchName: string, + description: string, + state: BranchStatus, + context?: string | null, +): Promise { + if (!is.nonEmptyString(context)) { + // already logged this case when validating the status check + return; + } + + await platform.setBranchStatus({ + branchName, + context, + description, + state, + }); +} + +interface ValidateReconfigureBranch { + status: boolean; +} +export async function validateReconfigureBranch( + config: RenovateConfig, + reconfigurePr: Pr | null, +): Promise { + logger.debug('validateReconfigureBranch()'); + + const context = config.statusCheckNames?.configValidation; + const branchName = getReconfigureBranchName(config.branchPrefix!); + + // look for config file + // 1. check reconfigure branch cache and use the configFileName if it exists + // 2. checkout reconfigure branch and look for the config file, don't assume default configFileName + const branchSha = getBranchCommit(branchName)!; + const cache = getCache(); + let configFileName: string | null = null; + const reconfigureCache = cache.reconfigureBranchCache; + // only use valid cached information + if (reconfigureCache?.reconfigureBranchSha === branchSha) { + logger.debug('Skipping validation check as branch sha is unchanged'); + return { status: reconfigureCache?.isConfigValid }; + } + + if (context) { + const validationStatus = await platform.getBranchStatusCheck( + branchName, + context, + ); + + // if old status check is present skip validation + if (is.nonEmptyString(validationStatus)) { + logger.debug( + 'Skipping validation check because status check already exists.', + ); + return { status: validationStatus === 'green' }; + } + } else { + logger.debug( + 'Status check is null or an empty string, skipping status check addition.', + ); + } + + try { + await scm.checkoutBranch(branchName); + configFileName = await detectConfigFile(); + } catch (err) { + logger.error( + { err }, + 'Error while searching for config file in reconfigure branch', + ); + } + + if (!is.nonEmptyString(configFileName)) { + logger.warn('No config file found in reconfigure branch'); + await setBranchStatus( + branchName, + 'Validation Failed - No config file found', + 'red', + context, + ); + setReconfigureBranchCache(branchSha, false); + await scm.checkoutBranch(config.defaultBranch!); + return { status: false }; + } + + let configFileRaw: string | null = null; + try { + configFileRaw = await readLocalFile(configFileName, 'utf8'); + } catch (err) { + logger.error({ err }, 'Error while reading config file'); + } + + if (!is.nonEmptyString(configFileRaw)) { + logger.warn('Empty or invalid config file'); + await setBranchStatus( + branchName, + 'Validation Failed - Empty/Invalid config file', + 'red', + context, + ); + setReconfigureBranchCache(branchSha, false); + await scm.checkoutBranch(config.baseBranch!); + return { status: false }; + } + + let configFileParsed: any; + try { + configFileParsed = JSON5.parse(configFileRaw); + // no need to confirm renovate field in package.json we already do it in `detectConfigFile()` + if (configFileName === 'package.json') { + configFileParsed = configFileParsed.renovate; + } + } catch (err) { + logger.error({ err }, 'Error while parsing config file'); + await setBranchStatus( + branchName, + 'Validation Failed - Unparsable config file', + 'red', + context, + ); + setReconfigureBranchCache(branchSha, false); + await scm.checkoutBranch(config.baseBranch!); + return { status: false }; + } + + // perform validation and provide a passing or failing check run based on result + const validationResult = await validateConfig('repo', configFileParsed); + + // failing check + // what happens when a status check is failed with errors and a comment is added but then the errors are resolved and a new commit is created .. is the comment deleted? test it + if (validationResult.errors.length > 0) { + logger.debug( + { errors: validationResult.errors.map((err) => err.message).join(', ') }, + 'Validation Errors', + ); + + // add comment to reconfigure PR if it exists + if (reconfigurePr) { + let body = `There is an error with this repository's Renovate configuration that needs to be fixed.\n\n`; + body += `Location: \`${configFileName}\`\n`; + body += `Message: \`${validationResult.errors + .map((e) => e.message) + .join(', ') + .replace(regEx(/`/g), "'")}\`\n`; + + await ensureComment({ + number: reconfigurePr.number, + topic: 'Action Required: Fix Renovate Configuration', + content: body, + }); + } + + await setBranchStatus(branchName, 'Validation Failed', 'red', context); + setReconfigureBranchCache(branchSha, false); + await scm.checkoutBranch(config.baseBranch!); + return { status: false }; + } + + // passing check + await setBranchStatus(branchName, 'Validation Successful', 'green', context); + + setReconfigureBranchCache(branchSha, true); + await scm.checkoutBranch(config.baseBranch!); + return { status: true }; +} From aa586b36383459d5cdd4c3acdc4080ff3073381e Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Mon, 30 Dec 2024 21:19:35 +0530 Subject: [PATCH 2/3] refactor tests --- .../repository/reconfigure/index.spec.ts | 222 +---------------- lib/workers/repository/reconfigure/index.ts | 30 +-- .../repository/reconfigure/validate.spec.ts | 228 ++++++++++++++++++ .../repository/reconfigure/validate.ts | 27 ++- 4 files changed, 255 insertions(+), 252 deletions(-) create mode 100644 lib/workers/repository/reconfigure/validate.spec.ts diff --git a/lib/workers/repository/reconfigure/index.spec.ts b/lib/workers/repository/reconfigure/index.spec.ts index ccde73c93e..52aff264f7 100644 --- a/lib/workers/repository/reconfigure/index.spec.ts +++ b/lib/workers/repository/reconfigure/index.spec.ts @@ -1,46 +1,29 @@ -import { mock } from 'jest-mock-extended'; import type { RenovateConfig } from '../../../../test/util'; -import { fs, git, mocked, partial, platform, scm } from '../../../../test/util'; +import { logger, mocked, scm } from '../../../../test/util'; import { GlobalConfig } from '../../../config/global'; -import { logger } from '../../../logger'; -import type { Pr } from '../../../modules/platform/types'; -import * as _cache from '../../../util/cache/repository'; -import type { LongCommitSha } from '../../../util/git/types'; -import * as _merge from '../init/merge'; +import * as _validate from './validate'; import { checkReconfigureBranch } from '.'; -jest.mock('../../../util/cache/repository'); -jest.mock('../../../util/fs'); -jest.mock('../../../util/git'); -jest.mock('../init/merge'); +jest.mock('./validate'); -const cache = mocked(_cache); -const merge = mocked(_merge); +const validate = mocked(_validate); describe('workers/repository/reconfigure/index', () => { const config: RenovateConfig = { branchPrefix: 'prefix/', baseBranch: 'base', - statusCheckNames: partial({ - configValidation: 'renovate/config-validation', - }), }; beforeEach(() => { - config.repository = 'some/repo'; - merge.detectConfigFile.mockResolvedValue('renovate.json'); - scm.branchExists.mockResolvedValue(true); - cache.getCache.mockReturnValue({}); - git.getBranchCommit.mockReturnValue('sha' as LongCommitSha); - fs.readLocalFile.mockResolvedValue(null); - platform.getBranchStatusCheck.mockResolvedValue(null); GlobalConfig.reset(); + scm.branchExists.mockResolvedValue(true); + validate.validateReconfigureBranch.mockResolvedValue(undefined); }); it('no effect when running with platform=local', async () => { GlobalConfig.set({ platform: 'local' }); await checkReconfigureBranch(config); - expect(logger.debug).toHaveBeenCalledWith( + expect(logger.logger.debug).toHaveBeenCalledWith( 'Not attempting to reconfigure when running with local platform', ); }); @@ -48,195 +31,12 @@ describe('workers/repository/reconfigure/index', () => { it('no effect on repo with no reconfigure branch', async () => { scm.branchExists.mockResolvedValueOnce(false); await checkReconfigureBranch(config); - expect(logger.debug).toHaveBeenCalledWith('No reconfigure branch found'); - }); - - it('logs error if config file search fails', async () => { - const err = new Error(); - merge.detectConfigFile.mockRejectedValueOnce(err as never); - await checkReconfigureBranch(config); - expect(logger.error).toHaveBeenCalledWith( - { err }, - 'Error while searching for config file in reconfigure branch', + expect(logger.logger.debug).toHaveBeenCalledWith( + 'No reconfigure branch found', ); }); - it('throws error if config file not found in reconfigure branch', async () => { - merge.detectConfigFile.mockResolvedValue(null); - await checkReconfigureBranch(config); - expect(logger.warn).toHaveBeenCalledWith( - 'No config file found in reconfigure branch', - ); - }); - - it('logs error if config file is unreadable', async () => { - const err = new Error(); - fs.readLocalFile.mockRejectedValueOnce(err as never); - await checkReconfigureBranch(config); - expect(logger.error).toHaveBeenCalledWith( - { err }, - 'Error while reading config file', - ); - }); - - it('throws error if config file is empty', async () => { - await checkReconfigureBranch(config); - expect(logger.warn).toHaveBeenCalledWith('Empty or invalid config file'); - }); - - it('throws error if config file content is invalid', async () => { - fs.readLocalFile.mockResolvedValueOnce(` - { - "name": - } - `); - await checkReconfigureBranch(config); - expect(logger.error).toHaveBeenCalledWith( - { err: expect.any(Object) }, - 'Error while parsing config file', - ); - expect(platform.setBranchStatus).toHaveBeenCalledWith({ - branchName: 'prefix/reconfigure', - context: 'renovate/config-validation', - description: 'Validation Failed - Unparsable config file', - state: 'red', - }); - }); - - it('handles failed validation', async () => { - fs.readLocalFile.mockResolvedValueOnce(` - { - "enabledManagers": ["docker"] - } - `); - await checkReconfigureBranch(config); - expect(logger.debug).toHaveBeenCalledWith( - { errors: expect.any(String) }, - 'Validation Errors', - ); - expect(platform.setBranchStatus).toHaveBeenCalledWith({ - branchName: 'prefix/reconfigure', - context: 'renovate/config-validation', - description: 'Validation Failed', - state: 'red', - }); - }); - - it('adds comment if reconfigure PR exists', async () => { - fs.readLocalFile.mockResolvedValueOnce(` - { - "enabledManagers": ["docker"] - } - `); - platform.findPr.mockResolvedValueOnce(mock({ number: 1 })); - await checkReconfigureBranch(config); - expect(logger.debug).toHaveBeenCalledWith( - { errors: expect.any(String) }, - 'Validation Errors', - ); - expect(platform.setBranchStatus).toHaveBeenCalled(); - expect(platform.ensureComment).toHaveBeenCalled(); - }); - - it('handles successful validation', async () => { - const pJson = ` - { - "renovate": { - "enabledManagers": ["npm"] - } - } - `; - merge.detectConfigFile.mockResolvedValue('package.json'); - fs.readLocalFile.mockResolvedValueOnce(pJson).mockResolvedValueOnce(pJson); - await checkReconfigureBranch(config); - expect(platform.setBranchStatus).toHaveBeenCalledWith({ - branchName: 'prefix/reconfigure', - context: 'renovate/config-validation', - description: 'Validation Successful', - state: 'green', - }); - }); - - it('skips adding status check if statusCheckNames.configValidation is null', async () => { - cache.getCache.mockReturnValueOnce({ - reconfigureBranchCache: { - reconfigureBranchSha: 'new-sha', - isConfigValid: false, - }, - }); - - await checkReconfigureBranch({ - ...config, - statusCheckNames: partial({ - configValidation: null, - }), - }); - expect(logger.debug).toHaveBeenCalledWith( - 'Status check is null or an empty string, skipping status check addition.', - ); - expect(platform.setBranchStatus).not.toHaveBeenCalled(); - }); - - it('skips adding status check if statusCheckNames.configValidation is empty string', async () => { - cache.getCache.mockReturnValueOnce({ - reconfigureBranchCache: { - reconfigureBranchSha: 'new-sha', - isConfigValid: false, - }, - }); - - await checkReconfigureBranch({ - ...config, - statusCheckNames: partial({ - configValidation: '', - }), - }); - expect(logger.debug).toHaveBeenCalledWith( - 'Status check is null or an empty string, skipping status check addition.', - ); - expect(platform.setBranchStatus).not.toHaveBeenCalled(); - }); - - it('skips validation if cache is valid', async () => { - cache.getCache.mockReturnValueOnce({ - reconfigureBranchCache: { - reconfigureBranchSha: 'sha', - isConfigValid: false, - }, - }); - await checkReconfigureBranch(config); - expect(logger.debug).toHaveBeenCalledWith( - 'Skipping validation check as branch sha is unchanged', - ); - }); - - it('skips validation if status check present', async () => { - cache.getCache.mockReturnValueOnce({ - reconfigureBranchCache: { - reconfigureBranchSha: 'new_sha', - isConfigValid: false, - }, - }); - platform.getBranchStatusCheck.mockResolvedValueOnce('green'); - await checkReconfigureBranch(config); - expect(logger.debug).toHaveBeenCalledWith( - 'Skipping validation check because status check already exists.', - ); - }); - - it('handles non-default config file', async () => { - merge.detectConfigFile.mockResolvedValue('.renovaterc'); - fs.readLocalFile.mockResolvedValueOnce(` - { - "enabledManagers": ["npm",] - } - `); - await checkReconfigureBranch(config); - expect(platform.setBranchStatus).toHaveBeenCalledWith({ - branchName: 'prefix/reconfigure', - context: 'renovate/config-validation', - description: 'Validation Successful', - state: 'green', - }); + it('validates reconfigure branch', async () => { + await expect(checkReconfigureBranch(config)).toResolve(); }); }); diff --git a/lib/workers/repository/reconfigure/index.ts b/lib/workers/repository/reconfigure/index.ts index 8143bd3919..abe55202eb 100644 --- a/lib/workers/repository/reconfigure/index.ts +++ b/lib/workers/repository/reconfigure/index.ts @@ -1,7 +1,6 @@ import { GlobalConfig } from '../../../config/global'; import type { RenovateConfig } from '../../../config/types'; import { logger } from '../../../logger'; -import { platform } from '../../../modules/platform'; import { scm } from '../../../modules/platform/scm'; import { deleteReconfigureBranchCache } from './reconfigure-cache'; import { getReconfigureBranchName } from './utils'; @@ -25,7 +24,7 @@ export async function checkReconfigureBranch( logger.debug('checkReconfigureBranch()'); if (GlobalConfig.get('platform') === 'local') { logger.debug( - 'Not attempting to check reconfigure branch when running with local platform', + 'Not attempting to reconfigure when running with local platform', ); return; } @@ -40,30 +39,5 @@ export async function checkReconfigureBranch( return; } - // check for the pr if it exists before hand so we do not need to make 2 calls - const reconfigurePr = await platform.findPr({ - branchName: reconfigureBranch, - state: 'open', - includeOtherAuthors: true, - }); - const { status } = await validateReconfigureBranch(config, reconfigurePr); - - // config is invalid someway - if (!status) { - logger.debug('Config is invalid skipping Pr creation'); - } - - // if status is true it mean 4 things - // 1. config file present - // 2. it has a valid config file name - // 3. it has valid json in its - // 4. the config in it is valid - - // now we need to check if a pr is present or not - // and if it is then, see if we can update anything in it. - // first test how an onboarding pr looks like and the info in it - // can be done by test run a repo or we can check the code - // remaining query is whether: the vaidate reconfigure branch needs to be refactored further? - // how the cache will be handled - // what happens if sha is same? > nothing cause we do not lookup so no sha change do nothing: unless a or is missing do created that + await validateReconfigureBranch(config); } diff --git a/lib/workers/repository/reconfigure/validate.spec.ts b/lib/workers/repository/reconfigure/validate.spec.ts new file mode 100644 index 0000000000..730bf75e37 --- /dev/null +++ b/lib/workers/repository/reconfigure/validate.spec.ts @@ -0,0 +1,228 @@ +import { mock } from 'jest-mock-extended'; +import type { RenovateConfig } from '../../../../test/util'; +import { fs, git, mocked, partial, platform, scm } from '../../../../test/util'; +import { GlobalConfig } from '../../../config/global'; +import { logger } from '../../../logger'; +import type { Pr } from '../../../modules/platform/types'; +import * as _cache from '../../../util/cache/repository'; +import type { LongCommitSha } from '../../../util/git/types'; +import * as _merge from '../init/merge'; +import { validateReconfigureBranch } from './validate'; + +jest.mock('../../../util/cache/repository'); +jest.mock('../../../util/fs'); +jest.mock('../../../util/git'); +jest.mock('../init/merge'); + +const cache = mocked(_cache); +const merge = mocked(_merge); + +describe('workers/repository/reconfigure/validate', () => { + const config: RenovateConfig = { + branchPrefix: 'prefix/', + baseBranch: 'base', + statusCheckNames: partial({ + configValidation: 'renovate/config-validation', + }), + }; + + beforeEach(() => { + config.repository = 'some/repo'; + merge.detectConfigFile.mockResolvedValue('renovate.json'); + scm.branchExists.mockResolvedValue(true); + cache.getCache.mockReturnValue({}); + git.getBranchCommit.mockReturnValue('sha' as LongCommitSha); + fs.readLocalFile.mockResolvedValue(null); + platform.getBranchStatusCheck.mockResolvedValue(null); + GlobalConfig.reset(); + }); + + it('logs error if config file search fails', async () => { + const err = new Error(); + merge.detectConfigFile.mockRejectedValueOnce(err as never); + await validateReconfigureBranch(config); + expect(logger.error).toHaveBeenCalledWith( + { err }, + 'Error while searching for config file in reconfigure branch', + ); + }); + + it('throws error if config file not found in reconfigure branch', async () => { + merge.detectConfigFile.mockResolvedValue(null); + await validateReconfigureBranch(config); + expect(logger.warn).toHaveBeenCalledWith( + 'No config file found in reconfigure branch', + ); + }); + + it('logs error if config file is unreadable', async () => { + const err = new Error(); + fs.readLocalFile.mockRejectedValueOnce(err as never); + await validateReconfigureBranch(config); + expect(logger.error).toHaveBeenCalledWith( + { err }, + 'Error while reading config file', + ); + }); + + it('throws error if config file is empty', async () => { + await validateReconfigureBranch(config); + expect(logger.warn).toHaveBeenCalledWith('Empty or invalid config file'); + }); + + it('throws error if config file content is invalid', async () => { + fs.readLocalFile.mockResolvedValueOnce(` + { + "name": + } + `); + await validateReconfigureBranch(config); + expect(logger.error).toHaveBeenCalledWith( + { err: expect.any(Object) }, + 'Error while parsing config file', + ); + expect(platform.setBranchStatus).toHaveBeenCalledWith({ + branchName: 'prefix/reconfigure', + context: 'renovate/config-validation', + description: 'Validation Failed - Unparsable config file', + state: 'red', + }); + }); + + it('handles failed validation', async () => { + fs.readLocalFile.mockResolvedValueOnce(` + { + "enabledManagers": ["docker"] + } + `); + await validateReconfigureBranch(config); + expect(logger.debug).toHaveBeenCalledWith( + { errors: expect.any(String) }, + 'Validation Errors', + ); + expect(platform.setBranchStatus).toHaveBeenCalledWith({ + branchName: 'prefix/reconfigure', + context: 'renovate/config-validation', + description: 'Validation Failed', + state: 'red', + }); + }); + + it('adds comment if reconfigure PR exists', async () => { + fs.readLocalFile.mockResolvedValueOnce(` + { + "enabledManagers": ["docker"] + } + `); + platform.findPr.mockResolvedValueOnce(mock({ number: 1 })); + await validateReconfigureBranch(config); + expect(logger.debug).toHaveBeenCalledWith( + { errors: expect.any(String) }, + 'Validation Errors', + ); + expect(platform.setBranchStatus).toHaveBeenCalled(); + expect(platform.ensureComment).toHaveBeenCalled(); + }); + + it('handles successful validation', async () => { + const pJson = ` + { + "renovate": { + "enabledManagers": ["npm"] + } + } + `; + merge.detectConfigFile.mockResolvedValue('package.json'); + fs.readLocalFile.mockResolvedValueOnce(pJson).mockResolvedValueOnce(pJson); + await validateReconfigureBranch(config); + expect(platform.setBranchStatus).toHaveBeenCalledWith({ + branchName: 'prefix/reconfigure', + context: 'renovate/config-validation', + description: 'Validation Successful', + state: 'green', + }); + }); + + it('skips adding status check if statusCheckNames.configValidation is null', async () => { + cache.getCache.mockReturnValueOnce({ + reconfigureBranchCache: { + reconfigureBranchSha: 'new-sha', + isConfigValid: false, + }, + }); + + await validateReconfigureBranch({ + ...config, + statusCheckNames: partial({ + configValidation: null, + }), + }); + expect(logger.debug).toHaveBeenCalledWith( + 'Status check is null or an empty string, skipping status check addition.', + ); + expect(platform.setBranchStatus).not.toHaveBeenCalled(); + }); + + it('skips adding status check if statusCheckNames.configValidation is empty string', async () => { + cache.getCache.mockReturnValueOnce({ + reconfigureBranchCache: { + reconfigureBranchSha: 'new-sha', + isConfigValid: false, + }, + }); + + await validateReconfigureBranch({ + ...config, + statusCheckNames: partial({ + configValidation: '', + }), + }); + expect(logger.debug).toHaveBeenCalledWith( + 'Status check is null or an empty string, skipping status check addition.', + ); + expect(platform.setBranchStatus).not.toHaveBeenCalled(); + }); + + it('skips validation if cache is valid', async () => { + cache.getCache.mockReturnValueOnce({ + reconfigureBranchCache: { + reconfigureBranchSha: 'sha', + isConfigValid: false, + }, + }); + await validateReconfigureBranch(config); + expect(logger.debug).toHaveBeenCalledWith( + 'Skipping validation check as branch sha is unchanged', + ); + }); + + it('skips validation if status check present', async () => { + cache.getCache.mockReturnValueOnce({ + reconfigureBranchCache: { + reconfigureBranchSha: 'new_sha', + isConfigValid: false, + }, + }); + platform.getBranchStatusCheck.mockResolvedValueOnce('green'); + await validateReconfigureBranch(config); + expect(logger.debug).toHaveBeenCalledWith( + 'Skipping validation check because status check already exists.', + ); + }); + + it('handles non-default config file', async () => { + merge.detectConfigFile.mockResolvedValue('.renovaterc'); + fs.readLocalFile.mockResolvedValueOnce(` + { + "enabledManagers": ["npm",] + } + `); + await validateReconfigureBranch(config); + expect(platform.setBranchStatus).toHaveBeenCalledWith({ + branchName: 'prefix/reconfigure', + context: 'renovate/config-validation', + description: 'Validation Successful', + state: 'green', + }); + }); +}); diff --git a/lib/workers/repository/reconfigure/validate.ts b/lib/workers/repository/reconfigure/validate.ts index ca05398614..e55a439187 100644 --- a/lib/workers/repository/reconfigure/validate.ts +++ b/lib/workers/repository/reconfigure/validate.ts @@ -3,7 +3,6 @@ import JSON5 from 'json5'; import type { RenovateConfig } from '../../../config/types'; import { validateConfig } from '../../../config/validation'; import { logger } from '../../../logger'; -import type { Pr } from '../../../modules/platform'; import { platform } from '../../../modules/platform'; import { ensureComment } from '../../../modules/platform/comment'; import { scm } from '../../../modules/platform/scm'; @@ -35,13 +34,9 @@ async function setBranchStatus( }); } -interface ValidateReconfigureBranch { - status: boolean; -} export async function validateReconfigureBranch( config: RenovateConfig, - reconfigurePr: Pr | null, -): Promise { +): Promise { logger.debug('validateReconfigureBranch()'); const context = config.statusCheckNames?.configValidation; @@ -57,7 +52,7 @@ export async function validateReconfigureBranch( // only use valid cached information if (reconfigureCache?.reconfigureBranchSha === branchSha) { logger.debug('Skipping validation check as branch sha is unchanged'); - return { status: reconfigureCache?.isConfigValid }; + return; } if (context) { @@ -71,7 +66,7 @@ export async function validateReconfigureBranch( logger.debug( 'Skipping validation check because status check already exists.', ); - return { status: validationStatus === 'green' }; + return; } } else { logger.debug( @@ -99,7 +94,7 @@ export async function validateReconfigureBranch( ); setReconfigureBranchCache(branchSha, false); await scm.checkoutBranch(config.defaultBranch!); - return { status: false }; + return; } let configFileRaw: string | null = null; @@ -119,7 +114,7 @@ export async function validateReconfigureBranch( ); setReconfigureBranchCache(branchSha, false); await scm.checkoutBranch(config.baseBranch!); - return { status: false }; + return; } let configFileParsed: any; @@ -139,7 +134,7 @@ export async function validateReconfigureBranch( ); setReconfigureBranchCache(branchSha, false); await scm.checkoutBranch(config.baseBranch!); - return { status: false }; + return; } // perform validation and provide a passing or failing check run based on result @@ -153,6 +148,12 @@ export async function validateReconfigureBranch( 'Validation Errors', ); + const reconfigurePr = await platform.findPr({ + branchName, + state: 'open', + includeOtherAuthors: true, + }); + // add comment to reconfigure PR if it exists if (reconfigurePr) { let body = `There is an error with this repository's Renovate configuration that needs to be fixed.\n\n`; @@ -172,7 +173,7 @@ export async function validateReconfigureBranch( await setBranchStatus(branchName, 'Validation Failed', 'red', context); setReconfigureBranchCache(branchSha, false); await scm.checkoutBranch(config.baseBranch!); - return { status: false }; + return; } // passing check @@ -180,5 +181,5 @@ export async function validateReconfigureBranch( setReconfigureBranchCache(branchSha, true); await scm.checkoutBranch(config.baseBranch!); - return { status: true }; + return; } From d79aa81503461e7766c1ec44da2710ff45a96009 Mon Sep 17 00:00:00 2001 From: Rahul Gautam Singh Date: Mon, 30 Dec 2024 21:22:56 +0530 Subject: [PATCH 3/3] refactor: remove comment --- lib/workers/repository/reconfigure/index.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/lib/workers/repository/reconfigure/index.ts b/lib/workers/repository/reconfigure/index.ts index abe55202eb..5977918c3b 100644 --- a/lib/workers/repository/reconfigure/index.ts +++ b/lib/workers/repository/reconfigure/index.ts @@ -6,18 +6,6 @@ import { deleteReconfigureBranchCache } from './reconfigure-cache'; import { getReconfigureBranchName } from './utils'; import { validateReconfigureBranch } from './validate'; -/** - * In a reconfigure branch: - * first check if such a branch exists (no->return) - * yes: - * check sha and only continue if sha doesn't match cached sha - * check if it has a valid config file (no failing check? and return) - * get the content, validate and give passing check as per it - * - * extractDeps - * ensure pr - */ - export async function checkReconfigureBranch( config: RenovateConfig, ): Promise {