From 5e8afe71e161fa1255632c5b9875e31a3a5374d7 Mon Sep 17 00:00:00 2001 From: Michael Kriese Date: Thu, 16 Sep 2021 10:35:30 +0200 Subject: [PATCH 1/3] fix(worker/branch): optimize artifact error handling --- lib/workers/branch/artifacts.spec.ts | 39 ++++++++++++++++++++++++++++ lib/workers/branch/artifacts.ts | 32 +++++++++++++++++++++++ lib/workers/branch/index.ts | 27 +++---------------- 3 files changed, 75 insertions(+), 23 deletions(-) create mode 100644 lib/workers/branch/artifacts.spec.ts create mode 100644 lib/workers/branch/artifacts.ts diff --git a/lib/workers/branch/artifacts.spec.ts b/lib/workers/branch/artifacts.spec.ts new file mode 100644 index 00000000000000..36e71ff96b897f --- /dev/null +++ b/lib/workers/branch/artifacts.spec.ts @@ -0,0 +1,39 @@ +import { getConfig, platform } from '../../../test/util'; +import { setGlobalConfig } from '../../config/global'; +import { BranchStatus } from '../../types'; +import { BranchConfig } from '../types'; +import { setArtifactsErrorStatus } from './artifacts'; + +const config: BranchConfig = { + ...getConfig(), + branchName: 'renovate/pin', + upgrades: [], +}; + +describe('workers/branch/artifacts', () => { + beforeEach(() => { + setGlobalConfig({}); + jest.resetAllMocks(); + }); + + describe('setArtifactsErrorStatus', () => { + it('adds status', async () => { + platform.getBranchStatusCheck.mockResolvedValueOnce(null); + await setArtifactsErrorStatus(config); + expect(platform.setBranchStatus).toHaveBeenCalled(); + }); + + it('skips status', async () => { + platform.getBranchStatusCheck.mockResolvedValueOnce(BranchStatus.red); + await setArtifactsErrorStatus(config); + expect(platform.setBranchStatus).not.toHaveBeenCalled(); + }); + + it('skips status (dry-run)', async () => { + setGlobalConfig({ dryRun: true }); + platform.getBranchStatusCheck.mockResolvedValueOnce(null); + await setArtifactsErrorStatus(config); + expect(platform.setBranchStatus).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/lib/workers/branch/artifacts.ts b/lib/workers/branch/artifacts.ts new file mode 100644 index 00000000000000..e9d51c37ab2d34 --- /dev/null +++ b/lib/workers/branch/artifacts.ts @@ -0,0 +1,32 @@ +import { getGlobalConfig } from '../../config/global'; +import { logger } from '../../logger'; +import { platform } from '../../platform'; +import { BranchStatus } from '../../types'; +import type { BranchConfig } from '../types'; + +export async function setArtifactsErrorStatus( + config: BranchConfig +): Promise { + const context = `renovate/artifacts`; + const description = 'Artifact file update failure'; + const state = BranchStatus.red; + const existingState = await platform.getBranchStatusCheck( + config.branchName, + context + ); + // Check if state needs setting + if (existingState !== state) { + console.log(existingState); + logger.debug(`Updating status check state to failed`); + if (getGlobalConfig().dryRun) { + logger.info('DRY-RUN: Would set branch status in ' + config.branchName); + } else { + await platform.setBranchStatus({ + branchName: config.branchName, + context, + description, + state, + }); + } + } +} diff --git a/lib/workers/branch/index.ts b/lib/workers/branch/index.ts index 96b33b94932d84..2e9d704e0c4cdb 100644 --- a/lib/workers/branch/index.ts +++ b/lib/workers/branch/index.ts @@ -36,6 +36,7 @@ import { Limit, isLimitReached } from '../global/limits'; import { ensurePr, getPlatformPrOptions } from '../pr'; import { checkAutoMerge } from '../pr/automerge'; import { BranchConfig, BranchResult, PrBlockedBy } from '../types'; +import { setArtifactsErrorStatus } from './artifacts'; import { tryBranchAutomerge } from './automerge'; import { prAlreadyExisted } from './check-existing'; import { commitFilesToBranch } from './commit'; @@ -393,6 +394,7 @@ export async function processBranch( removeMeta(['dep']); if (config.artifactErrors?.length) { + await setArtifactsErrorStatus(config); if (config.releaseTimestamp) { logger.debug(`Branch timestamp: ` + config.releaseTimestamp); const releaseTimestamp = DateTime.fromISO(config.releaseTimestamp); @@ -460,7 +462,9 @@ export async function processBranch( await setConfidence(config); // break if we pushed a new commit because status check are pretty sure pending but maybe not reported yet + // but do not break when there are artifact errors if ( + !config.artifactErrors?.length && !dependencyDashboardCheck && !config.rebaseRequested && commitSha && @@ -669,29 +673,6 @@ export async function processBranch( }); } } - const context = `renovate/artifacts`; - const description = 'Artifact file update failure'; - const state = BranchStatus.red; - const existingState = await platform.getBranchStatusCheck( - config.branchName, - context - ); - // Check if state needs setting - if (existingState !== state) { - logger.debug(`Updating status check state to failed`); - if (getGlobalConfig().dryRun) { - logger.info( - 'DRY-RUN: Would set branch status in ' + config.branchName - ); - } else { - await platform.setBranchStatus({ - branchName: config.branchName, - context, - description, - state, - }); - } - } } else if (config.automerge) { logger.debug('PR is configured for automerge'); const prAutomergeResult = await checkAutoMerge(pr, config); From e33b292f3d7c7bf208fcb07a4c9f135f9b8e1126 Mon Sep 17 00:00:00 2001 From: Michael Kriese Date: Thu, 16 Sep 2021 10:39:06 +0200 Subject: [PATCH 2/3] chore: cleanup --- lib/workers/branch/artifacts.spec.ts | 8 ++++---- lib/workers/branch/artifacts.ts | 3 +-- lib/workers/branch/index.ts | 4 ++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/workers/branch/artifacts.spec.ts b/lib/workers/branch/artifacts.spec.ts index 36e71ff96b897f..d06d229616d27c 100644 --- a/lib/workers/branch/artifacts.spec.ts +++ b/lib/workers/branch/artifacts.spec.ts @@ -2,7 +2,7 @@ import { getConfig, platform } from '../../../test/util'; import { setGlobalConfig } from '../../config/global'; import { BranchStatus } from '../../types'; import { BranchConfig } from '../types'; -import { setArtifactsErrorStatus } from './artifacts'; +import { setArtifactErrorStatus } from './artifacts'; const config: BranchConfig = { ...getConfig(), @@ -19,20 +19,20 @@ describe('workers/branch/artifacts', () => { describe('setArtifactsErrorStatus', () => { it('adds status', async () => { platform.getBranchStatusCheck.mockResolvedValueOnce(null); - await setArtifactsErrorStatus(config); + await setArtifactErrorStatus(config); expect(platform.setBranchStatus).toHaveBeenCalled(); }); it('skips status', async () => { platform.getBranchStatusCheck.mockResolvedValueOnce(BranchStatus.red); - await setArtifactsErrorStatus(config); + await setArtifactErrorStatus(config); expect(platform.setBranchStatus).not.toHaveBeenCalled(); }); it('skips status (dry-run)', async () => { setGlobalConfig({ dryRun: true }); platform.getBranchStatusCheck.mockResolvedValueOnce(null); - await setArtifactsErrorStatus(config); + await setArtifactErrorStatus(config); expect(platform.setBranchStatus).not.toHaveBeenCalled(); }); }); diff --git a/lib/workers/branch/artifacts.ts b/lib/workers/branch/artifacts.ts index e9d51c37ab2d34..2200faf0f72230 100644 --- a/lib/workers/branch/artifacts.ts +++ b/lib/workers/branch/artifacts.ts @@ -4,7 +4,7 @@ import { platform } from '../../platform'; import { BranchStatus } from '../../types'; import type { BranchConfig } from '../types'; -export async function setArtifactsErrorStatus( +export async function setArtifactErrorStatus( config: BranchConfig ): Promise { const context = `renovate/artifacts`; @@ -16,7 +16,6 @@ export async function setArtifactsErrorStatus( ); // Check if state needs setting if (existingState !== state) { - console.log(existingState); logger.debug(`Updating status check state to failed`); if (getGlobalConfig().dryRun) { logger.info('DRY-RUN: Would set branch status in ' + config.branchName); diff --git a/lib/workers/branch/index.ts b/lib/workers/branch/index.ts index 2e9d704e0c4cdb..1e3f8127e7ef19 100644 --- a/lib/workers/branch/index.ts +++ b/lib/workers/branch/index.ts @@ -36,7 +36,7 @@ import { Limit, isLimitReached } from '../global/limits'; import { ensurePr, getPlatformPrOptions } from '../pr'; import { checkAutoMerge } from '../pr/automerge'; import { BranchConfig, BranchResult, PrBlockedBy } from '../types'; -import { setArtifactsErrorStatus } from './artifacts'; +import { setArtifactErrorStatus } from './artifacts'; import { tryBranchAutomerge } from './automerge'; import { prAlreadyExisted } from './check-existing'; import { commitFilesToBranch } from './commit'; @@ -394,7 +394,7 @@ export async function processBranch( removeMeta(['dep']); if (config.artifactErrors?.length) { - await setArtifactsErrorStatus(config); + await setArtifactErrorStatus(config); if (config.releaseTimestamp) { logger.debug(`Branch timestamp: ` + config.releaseTimestamp); const releaseTimestamp = DateTime.fromISO(config.releaseTimestamp); From 7ae1f8f9f19c9326cbab1ade2bfe9993543f92a5 Mon Sep 17 00:00:00 2001 From: Michael Kriese Date: Thu, 16 Sep 2021 15:39:32 +0200 Subject: [PATCH 3/3] fix: move status assiggnemnt --- lib/workers/branch/artifacts.spec.ts | 20 ++++++++++++++------ lib/workers/branch/artifacts.ts | 6 ++++++ lib/workers/branch/index.ts | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/lib/workers/branch/artifacts.spec.ts b/lib/workers/branch/artifacts.spec.ts index d06d229616d27c..d185d13c0611d0 100644 --- a/lib/workers/branch/artifacts.spec.ts +++ b/lib/workers/branch/artifacts.spec.ts @@ -4,16 +4,17 @@ import { BranchStatus } from '../../types'; import { BranchConfig } from '../types'; import { setArtifactErrorStatus } from './artifacts'; -const config: BranchConfig = { - ...getConfig(), - branchName: 'renovate/pin', - upgrades: [], -}; - describe('workers/branch/artifacts', () => { + let config: BranchConfig; beforeEach(() => { setGlobalConfig({}); jest.resetAllMocks(); + config = { + ...getConfig(), + branchName: 'renovate/pin', + upgrades: [], + artifactErrors: [{ lockFile: 'some' }], + }; }); describe('setArtifactsErrorStatus', () => { @@ -35,5 +36,12 @@ describe('workers/branch/artifacts', () => { await setArtifactErrorStatus(config); expect(platform.setBranchStatus).not.toHaveBeenCalled(); }); + + it('skips status (no errors)', async () => { + platform.getBranchStatusCheck.mockResolvedValueOnce(null); + config.artifactErrors.length = 0; + await setArtifactErrorStatus(config); + expect(platform.setBranchStatus).not.toHaveBeenCalled(); + }); }); }); diff --git a/lib/workers/branch/artifacts.ts b/lib/workers/branch/artifacts.ts index 2200faf0f72230..6c982bb99cee5a 100644 --- a/lib/workers/branch/artifacts.ts +++ b/lib/workers/branch/artifacts.ts @@ -7,6 +7,11 @@ import type { BranchConfig } from '../types'; export async function setArtifactErrorStatus( config: BranchConfig ): Promise { + if (!config.artifactErrors?.length) { + // no errors + return; + } + const context = `renovate/artifacts`; const description = 'Artifact file update failure'; const state = BranchStatus.red; @@ -14,6 +19,7 @@ export async function setArtifactErrorStatus( config.branchName, context ); + // Check if state needs setting if (existingState !== state) { logger.debug(`Updating status check state to failed`); diff --git a/lib/workers/branch/index.ts b/lib/workers/branch/index.ts index 1e3f8127e7ef19..da893445930b4a 100644 --- a/lib/workers/branch/index.ts +++ b/lib/workers/branch/index.ts @@ -394,7 +394,6 @@ export async function processBranch( removeMeta(['dep']); if (config.artifactErrors?.length) { - await setArtifactErrorStatus(config); if (config.releaseTimestamp) { logger.debug(`Branch timestamp: ` + config.releaseTimestamp); const releaseTimestamp = DateTime.fromISO(config.releaseTimestamp); @@ -458,6 +457,7 @@ export async function processBranch( logger.info({ commitSha }, `Branch ${action}`); } // Set branch statuses + await setArtifactErrorStatus(config); await setStability(config); await setConfidence(config);