diff --git a/lib/workers/branch/index.js b/lib/workers/branch/index.js index 496eb636c030867772de1730c12b5f252808e2ef..9e0822ee529655c9a22ee31efd1b1791dc942258 100644 --- a/lib/workers/branch/index.js +++ b/lib/workers/branch/index.js @@ -79,9 +79,6 @@ async function processBranch(branchConfig) { logger.debug('No package files need updating'); } Object.assign(config, await getUpdatedLockFiles(config)); - if (config.lockFileError) { - return 'lockFileError'; - } if (config.updatedLockFiles.length) { logger.debug( { updatedLockFiles: config.updatedLockFiles }, @@ -124,9 +121,41 @@ async function processBranch(branchConfig) { const pr = await prWorker.ensurePr(config); // TODO: ensurePr should check for automerge itself if (pr) { - const prAutomerged = await prWorker.checkAutoMerge(pr, config); - if (prAutomerged) { - return 'automerged'; + const topic = 'Lock file problem'; + if (config.lockFileErrors && config.lockFileErrors.length) { + logger.warn( + { lockFileErrors: config.lockFileErrors }, + 'lockFileErrors' + ); + let content = `Renovate failed when attempting to generate `; + content += + config.lockFileErrors.length > 1 ? 'lock files' : 'a lock file'; + content += + '. This is usually happens when you have private modules but have not added configuration for [private module support](https://renovateapp.com/docs/deep-dives/private-modules). It is strongly recommended that you do not merge this PR as-is.'; + content += + '\n\nRenovate **will not retry** generating a lockfile for this PR unless either (a) the `package.json` in this branch needs updating, or (b) '; + if (config.recreateClosed) { + content += + 'you manually delete this PR so that it can be regenerated.'; + } else { + content += + 'you rename then delete this PR unmerged, so that it can be regenerated.'; + } + content += '\n\nThe output from `stderr` is included below:'; + const subtopics = []; + config.lockFileErrors.forEach(error => { + subtopics.push({ + topic: error.lockFile, + content: `\`\`\`\n${error.stderr}\n\`\`\``, + }); + }); + await config.api.ensureComment(pr.number, topic, content, subtopics); + } else { + await config.api.ensureCommentRemoval(pr.number, topic); + const prAutomerged = await prWorker.checkAutoMerge(pr, config); + if (prAutomerged) { + return 'automerged'; + } } } } catch (err) { diff --git a/lib/workers/branch/lock-files.js b/lib/workers/branch/lock-files.js index 586897e9b5366772443dbfcc397d3ae63c851008..4b1de0d28d0196ed6943f088a1601b1d5f96e7a1 100644 --- a/lib/workers/branch/lock-files.js +++ b/lib/workers/branch/lock-files.js @@ -194,74 +194,75 @@ async function getUpdatedLockFiles(config) { const { logger } = config; logger.trace({ config }, 'getUpdatedLockFiles'); logger.debug('Getting updated lock files'); - let lockFileError = false; + const lockFileErrors = []; const updatedLockFiles = []; - try { - if ( - config.type === 'lockFileMaintenance' && - (await config.api.branchExists(config.branchName)) - ) { - return { lockFileError, updatedLockFiles }; - } - const dirs = module.exports.determineLockFileDirs(config); - logger.debug({ dirs }, 'lock file dirs'); - await module.exports.writeExistingFiles(config); - await module.exports.writeUpdatedPackageFiles(config); + if ( + config.type === 'lockFileMaintenance' && + (await config.api.branchExists(config.branchName)) + ) { + return { lockFileErrors, updatedLockFiles }; + } + const dirs = module.exports.determineLockFileDirs(config); + logger.debug({ dirs }, 'lock file dirs'); + await module.exports.writeExistingFiles(config); + await module.exports.writeUpdatedPackageFiles(config); - for (const lockFileDir of dirs.packageLockFileDirs) { - logger.debug(`Generating package-lock.json for ${lockFileDir}`); - const newContent = await npm.generateLockFile( - path.join(config.tmpDir.path, lockFileDir), - logger + for (const lockFileDir of dirs.packageLockFileDirs) { + logger.debug(`Generating package-lock.json for ${lockFileDir}`); + const lockFileName = path.join(lockFileDir, 'package-lock.json'); + const res = await npm.generateLockFile( + path.join(config.tmpDir.path, lockFileDir), + logger + ); + if (res.error) { + lockFileErrors.push({ + lockFile: lockFileName, + stderr: res.stderr, + }); + } else { + const existingContent = await config.api.getFileContent( + lockFileName, + config.parentBranch ); - if (newContent) { - const lockFileName = path.join(lockFileDir, 'package-lock.json'); - const existingContent = await config.api.getFileContent( - lockFileName, - config.parentBranch - ); - if (newContent !== existingContent) { - logger.debug('package-lock.json needs updating'); - updatedLockFiles.push({ - name: lockFileName, - contents: newContent, - }); - } else { - logger.debug("package-lock.json hasn't changed"); - } + if (res.lockFile !== existingContent) { + logger.debug('package-lock.json needs updating'); + updatedLockFiles.push({ + name: lockFileName, + contents: res.lockFile, + }); } else { - lockFileError = true; + logger.debug("package-lock.json hasn't changed"); } } + } - for (const lockFileDir of dirs.yarnLockFileDirs) { - logger.debug(`Generating yarn.lock for ${lockFileDir}`); - const newContent = await yarn.generateLockFile( - path.join(config.tmpDir.path, lockFileDir), - logger + for (const lockFileDir of dirs.yarnLockFileDirs) { + logger.debug(`Generating yarn.lock for ${lockFileDir}`); + const lockFileName = path.join(lockFileDir, 'yarn.lock'); + const res = await yarn.generateLockFile( + path.join(config.tmpDir.path, lockFileDir), + logger + ); + if (res.error) { + lockFileErrors.push({ + lockFile: lockFileName, + stderr: res.stderr, + }); + } else { + const existingContent = await config.api.getFileContent( + lockFileName, + config.parentBranch ); - if (newContent) { - const lockFileName = path.join(lockFileDir, 'yarn.lock'); - const existingContent = await config.api.getFileContent( - lockFileName, - config.parentBranch - ); - if (newContent !== existingContent) { - logger.debug('yarn.lock needs updating'); - updatedLockFiles.push({ - name: lockFileName, - contents: newContent, - }); - } else { - logger.debug("yarn.lock hasn't changed"); - } + if (res.lockFile !== existingContent) { + logger.debug('yarn.lock needs updating'); + updatedLockFiles.push({ + name: lockFileName, + contents: res.lockFile, + }); } else { - lockFileError = true; + logger.debug("yarn.lock hasn't changed"); } } - } catch (err) { - logger.error({ err }, 'getUpdatedLockFiles error'); - lockFileError = true; } - return { lockFileError, updatedLockFiles }; + return { lockFileErrors, updatedLockFiles }; } diff --git a/lib/workers/branch/npm.js b/lib/workers/branch/npm.js index 599432b32452f22ebbc7236f5c95c2dce72c0a71..be19e2f99b9433bd1cf9580d1c036c96250c2ed2 100644 --- a/lib/workers/branch/npm.js +++ b/lib/workers/branch/npm.js @@ -82,6 +82,7 @@ async function generateLockFile(tmpDir, logger) { }, 'npm install error' ); + return { error: true, stderr }; } - return lockFile; + return { lockFile }; } diff --git a/lib/workers/branch/yarn.js b/lib/workers/branch/yarn.js index 51b06610349b9bb0658de56c5e223a4b725b56da..ebcc43527010bc9d364aef314fcd4ada580b57d0 100644 --- a/lib/workers/branch/yarn.js +++ b/lib/workers/branch/yarn.js @@ -72,7 +72,7 @@ async function generateLockFile(tmpDir, logger) { 'Generated lockfile' ); } catch (err) /* istanbul ignore next */ { - logger.warn( + logger.info( { err, stdout, @@ -80,6 +80,7 @@ async function generateLockFile(tmpDir, logger) { }, 'yarn install error' ); + return { error: true, stderr: err.stderr }; } - return lockFile; + return { lockFile }; } diff --git a/test/workers/branch/__snapshots__/lock-files.spec.js.snap b/test/workers/branch/__snapshots__/lock-files.spec.js.snap index bfe66104c91c50953f73f988ff2cfc55606a769e..7fc024e95dee56e5672c5708832e38666623ea1f 100644 --- a/test/workers/branch/__snapshots__/lock-files.spec.js.snap +++ b/test/workers/branch/__snapshots__/lock-files.spec.js.snap @@ -33,14 +33,14 @@ Object { exports[`workers/branch/lock-files getUpdatedLockFiles adds multiple lock files 1`] = ` Object { - "lockFileError": false, + "lockFileErrors": Array [], "updatedLockFiles": Array [ Object { - "contents": "some new lock file contents", + "contents": undefined, "name": "a/package-lock.json", }, Object { - "contents": "some new lock file contents", + "contents": undefined, "name": "c/yarn.lock", }, ], @@ -49,42 +49,37 @@ Object { exports[`workers/branch/lock-files getUpdatedLockFiles returns no error and empty lockfiles if lock file maintenance exists 1`] = ` Object { - "lockFileError": false, + "lockFileErrors": Array [], "updatedLockFiles": Array [], } `; exports[`workers/branch/lock-files getUpdatedLockFiles returns no error and empty lockfiles if none updated 1`] = ` Object { - "lockFileError": false, - "updatedLockFiles": Array [], -} -`; - -exports[`workers/branch/lock-files getUpdatedLockFiles returns npm errors 1`] = ` -Object { - "lockFileError": true, - "updatedLockFiles": Array [], -} -`; - -exports[`workers/branch/lock-files getUpdatedLockFiles returns yarn errors 1`] = ` -Object { - "lockFileError": true, + "lockFileErrors": Array [], "updatedLockFiles": Array [], } `; exports[`workers/branch/lock-files getUpdatedLockFiles sets error if receiving null 1`] = ` Object { - "lockFileError": true, + "lockFileErrors": Array [ + Object { + "lockFile": "a/package-lock.json", + "stderr": undefined, + }, + Object { + "lockFile": "c/yarn.lock", + "stderr": undefined, + }, + ], "updatedLockFiles": Array [], } `; exports[`workers/branch/lock-files getUpdatedLockFiles tries multiple lock files 1`] = ` Object { - "lockFileError": false, + "lockFileErrors": Array [], "updatedLockFiles": Array [], } `; diff --git a/test/workers/branch/index.spec.js b/test/workers/branch/index.spec.js index fdcb91b06db34249c464ba44a769edaa25c1d510..bc45bf51e1fbf7018add250ff9f65afb870a34e2 100644 --- a/test/workers/branch/index.spec.js +++ b/test/workers/branch/index.spec.js @@ -27,9 +27,15 @@ describe('workers/branch', () => { describe('processBranch', () => { let config; beforeEach(() => { + prWorker.ensurePr = jest.fn(); + prWorker.checkAutoMerge = jest.fn(); config = { ...defaultConfig, - api: { branchExists: jest.fn(), ensureComment: jest.fn() }, + api: { + branchExists: jest.fn(), + ensureComment: jest.fn(), + ensureCommentRemoval: jest.fn(), + }, errors: [], warnings: [], logger, @@ -101,8 +107,44 @@ describe('workers/branch', () => { prWorker.checkAutoMerge.mockReturnValueOnce(true); await branchWorker.processBranch(config); expect(prWorker.ensurePr.mock.calls).toHaveLength(1); + expect(config.api.ensureCommentRemoval.mock.calls).toHaveLength(1); expect(prWorker.checkAutoMerge.mock.calls).toHaveLength(1); }); + it('ensures PR and adds lock file error comment', async () => { + packageFiles.getUpdatedPackageFiles.mockReturnValueOnce([{}]); + lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ + lockFileError: false, + updatedLockFiles: [{}], + }); + config.api.branchExists.mockReturnValueOnce(true); + automerge.tryBranchAutomerge.mockReturnValueOnce('failed'); + prWorker.ensurePr.mockReturnValueOnce({}); + prWorker.checkAutoMerge.mockReturnValueOnce(true); + config.lockFileErrors = [{}]; + await branchWorker.processBranch(config); + expect(config.api.ensureComment.mock.calls).toHaveLength(1); + expect(config.api.ensureCommentRemoval.mock.calls).toHaveLength(0); + expect(prWorker.ensurePr.mock.calls).toHaveLength(1); + expect(prWorker.checkAutoMerge.mock.calls).toHaveLength(0); + }); + it('ensures PR and adds lock file error comment recreate closed', async () => { + packageFiles.getUpdatedPackageFiles.mockReturnValueOnce([{}]); + lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ + lockFileError: false, + updatedLockFiles: [{}], + }); + config.recreateClosed = true; + config.api.branchExists.mockReturnValueOnce(true); + automerge.tryBranchAutomerge.mockReturnValueOnce('failed'); + prWorker.ensurePr.mockReturnValueOnce({}); + prWorker.checkAutoMerge.mockReturnValueOnce(true); + config.lockFileErrors = [{}]; + await branchWorker.processBranch(config); + expect(config.api.ensureComment.mock.calls).toHaveLength(1); + expect(config.api.ensureCommentRemoval.mock.calls).toHaveLength(0); + expect(prWorker.ensurePr.mock.calls).toHaveLength(1); + expect(prWorker.checkAutoMerge.mock.calls).toHaveLength(0); + }); it('swallows branch errors', async () => { packageFiles.getUpdatedPackageFiles.mockImplementationOnce(() => { throw new Error('some error'); diff --git a/test/workers/branch/lock-files.spec.js b/test/workers/branch/lock-files.spec.js index cbec321bb0e411135d17ceca4b3144b5ef86c745..7bbcdabe93722533ea9d9b9ee6ddbfb80bd88ec2 100644 --- a/test/workers/branch/lock-files.spec.js +++ b/test/workers/branch/lock-files.spec.js @@ -284,9 +284,13 @@ describe('workers/branch/lock-files', () => { tmpDir: { path: 'some-tmp-dir' }, }; npm.generateLockFile = jest.fn(); - npm.generateLockFile.mockReturnValue('some lock file contents'); + npm.generateLockFile.mockReturnValue({ + lockFile: 'some lock file contents', + }); yarn.generateLockFile = jest.fn(); - yarn.generateLockFile.mockReturnValue('some lock file contents'); + yarn.generateLockFile.mockReturnValue({ + lockFile: 'some lock file contents', + }); lockFiles.determineLockFileDirs = jest.fn(); }); it('returns no error and empty lockfiles if lock file maintenance exists', async () => { @@ -294,7 +298,7 @@ describe('workers/branch/lock-files', () => { config.api.branchExists.mockReturnValueOnce(true); const res = await getUpdatedLockFiles(config); expect(res).toMatchSnapshot(); - expect(res.lockFileError).toBe(false); + expect(res.lockFileErrors).toHaveLength(0); expect(res.updatedLockFiles).toHaveLength(0); }); it('returns no error and empty lockfiles if none updated', async () => { @@ -304,7 +308,7 @@ describe('workers/branch/lock-files', () => { }); const res = await getUpdatedLockFiles(config); expect(res).toMatchSnapshot(); - expect(res.lockFileError).toBe(false); + expect(res.lockFileErrors).toHaveLength(0); expect(res.updatedLockFiles).toHaveLength(0); }); it('tries multiple lock files', async () => { @@ -314,7 +318,7 @@ describe('workers/branch/lock-files', () => { }); const res = await getUpdatedLockFiles(config); expect(res).toMatchSnapshot(); - expect(res.lockFileError).toBe(false); + expect(res.lockFileErrors).toHaveLength(0); expect(res.updatedLockFiles).toHaveLength(0); expect(npm.generateLockFile.mock.calls).toHaveLength(2); expect(yarn.generateLockFile.mock.calls).toHaveLength(2); @@ -325,11 +329,11 @@ describe('workers/branch/lock-files', () => { packageLockFileDirs: ['a', 'b'], yarnLockFileDirs: ['c', 'd'], }); - npm.generateLockFile.mockReturnValueOnce(null); - yarn.generateLockFile.mockReturnValueOnce(null); + npm.generateLockFile.mockReturnValueOnce({ error: true }); + yarn.generateLockFile.mockReturnValueOnce({ error: true }); const res = await getUpdatedLockFiles(config); expect(res).toMatchSnapshot(); - expect(res.lockFileError).toBe(true); + expect(res.lockFileErrors).toHaveLength(2); expect(res.updatedLockFiles).toHaveLength(0); expect(npm.generateLockFile.mock.calls).toHaveLength(2); expect(yarn.generateLockFile.mock.calls).toHaveLength(2); @@ -344,43 +348,11 @@ describe('workers/branch/lock-files', () => { yarn.generateLockFile.mockReturnValueOnce('some new lock file contents'); const res = await getUpdatedLockFiles(config); expect(res).toMatchSnapshot(); - expect(res.lockFileError).toBe(false); + expect(res.lockFileErrors).toHaveLength(0); expect(res.updatedLockFiles).toHaveLength(2); expect(npm.generateLockFile.mock.calls).toHaveLength(2); expect(yarn.generateLockFile.mock.calls).toHaveLength(2); expect(config.api.getFileContent.mock.calls).toHaveLength(4); }); - it('returns npm errors', async () => { - lockFiles.determineLockFileDirs.mockReturnValueOnce({ - packageLockFileDirs: ['a', 'b'], - yarnLockFileDirs: ['c', 'd'], - }); - npm.generateLockFile.mockImplementationOnce(() => { - throw new Error('some error'); - }); - const res = await getUpdatedLockFiles(config); - expect(res).toMatchSnapshot(); - expect(res.lockFileError).toBe(true); - expect(res.updatedLockFiles).toHaveLength(0); - expect(npm.generateLockFile.mock.calls).toHaveLength(1); - expect(yarn.generateLockFile.mock.calls).toHaveLength(0); - expect(config.api.getFileContent.mock.calls).toHaveLength(0); - }); - it('returns yarn errors', async () => { - lockFiles.determineLockFileDirs.mockReturnValueOnce({ - packageLockFileDirs: [], - yarnLockFileDirs: ['c', 'd'], - }); - yarn.generateLockFile.mockImplementationOnce(() => { - throw new Error('some error'); - }); - const res = await getUpdatedLockFiles(config); - expect(res).toMatchSnapshot(); - expect(res.lockFileError).toBe(true); - expect(res.updatedLockFiles).toHaveLength(0); - expect(npm.generateLockFile.mock.calls).toHaveLength(0); - expect(yarn.generateLockFile.mock.calls).toHaveLength(1); - expect(config.api.getFileContent.mock.calls).toHaveLength(0); - }); }); }); diff --git a/test/workers/branch/npm.spec.js b/test/workers/branch/npm.spec.js index b5319b7e37b21f145d889f54900dc709541ea8c4..7001c778bca4fa253043bb681ce48829a368da2d 100644 --- a/test/workers/branch/npm.spec.js +++ b/test/workers/branch/npm.spec.js @@ -19,9 +19,10 @@ describe('generateLockFile', () => { stderror: '', }); fs.readFile = jest.fn(() => 'package-lock-contents'); - const lockFile = await npmHelper.generateLockFile('some-dir', logger); + const res = await npmHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toEqual('package-lock-contents'); + expect(res.error).not.toBeDefined(); + expect(res.lockFile).toEqual('package-lock-contents'); }); it('catches errors', async () => { getInstalledPath.mockReturnValueOnce('node_modules/npm'); @@ -32,9 +33,10 @@ describe('generateLockFile', () => { fs.readFile = jest.fn(() => { throw new Error('not found'); }); - const lockFile = await npmHelper.generateLockFile('some-dir', logger); + const res = await npmHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toBe(null); + expect(res.error).toBe(true); + expect(res.lockFile).not.toBeDefined(); }); it('finds npm embedded in renovate', async () => { getInstalledPath.mockImplementationOnce(() => { @@ -49,9 +51,9 @@ describe('generateLockFile', () => { stderror: '', }); fs.readFile = jest.fn(() => 'package-lock-contents'); - const lockFile = await npmHelper.generateLockFile('some-dir', logger); + const res = await npmHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toEqual('package-lock-contents'); + expect(res.lockFile).toEqual('package-lock-contents'); }); it('finds npm globally', async () => { getInstalledPath.mockImplementationOnce(() => { @@ -67,9 +69,9 @@ describe('generateLockFile', () => { stderror: '', }); fs.readFile = jest.fn(() => 'package-lock-contents'); - const lockFile = await npmHelper.generateLockFile('some-dir', logger); + const res = await npmHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toEqual('package-lock-contents'); + expect(res.lockFile).toEqual('package-lock-contents'); }); it('uses fallback npm', async () => { getInstalledPath.mockImplementationOnce(() => { @@ -87,8 +89,8 @@ describe('generateLockFile', () => { stderror: '', }); fs.readFile = jest.fn(() => 'package-lock-contents'); - const lockFile = await npmHelper.generateLockFile('some-dir', logger); + const res = await npmHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toEqual('package-lock-contents'); + expect(res.lockFile).toEqual('package-lock-contents'); }); }); diff --git a/test/workers/branch/yarn.spec.js b/test/workers/branch/yarn.spec.js index 8cfb1bcc64b8618a88694ede181328dcd90fff1a..d82e139b7e0c29c29b430b480224dfa809a84882 100644 --- a/test/workers/branch/yarn.spec.js +++ b/test/workers/branch/yarn.spec.js @@ -19,9 +19,9 @@ describe('generateLockFile', () => { stderror: '', }); fs.readFile = jest.fn(() => 'package-lock-contents'); - const lockFile = await yarnHelper.generateLockFile('some-dir', logger); + const res = await yarnHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toEqual('package-lock-contents'); + expect(res.lockFile).toEqual('package-lock-contents'); }); it('catches errors', async () => { getInstalledPath.mockReturnValueOnce('node_modules/yarn'); @@ -32,9 +32,10 @@ describe('generateLockFile', () => { fs.readFile = jest.fn(() => { throw new Error('not found'); }); - const lockFile = await yarnHelper.generateLockFile('some-dir', logger); + const res = await yarnHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toBe(null); + expect(res.error).toBe(true); + expect(res.lockFile).not.toBeDefined(); }); it('finds yarn embedded in renovate', async () => { getInstalledPath.mockImplementationOnce(() => { @@ -49,9 +50,9 @@ describe('generateLockFile', () => { stderror: '', }); fs.readFile = jest.fn(() => 'package-lock-contents'); - const lockFile = await yarnHelper.generateLockFile('some-dir', logger); + const res = await yarnHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toEqual('package-lock-contents'); + expect(res.lockFile).toEqual('package-lock-contents'); }); it('finds yarn globally', async () => { getInstalledPath.mockImplementationOnce(() => { @@ -67,9 +68,9 @@ describe('generateLockFile', () => { stderror: '', }); fs.readFile = jest.fn(() => 'package-lock-contents'); - const lockFile = await yarnHelper.generateLockFile('some-dir', logger); + const res = await yarnHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toEqual('package-lock-contents'); + expect(res.lockFile).toEqual('package-lock-contents'); }); it('uses fallback yarn', async () => { getInstalledPath.mockImplementationOnce(() => { @@ -87,8 +88,8 @@ describe('generateLockFile', () => { stderror: '', }); fs.readFile = jest.fn(() => 'package-lock-contents'); - const lockFile = await yarnHelper.generateLockFile('some-dir', logger); + const res = await yarnHelper.generateLockFile('some-dir', logger); expect(fs.readFile.mock.calls.length).toEqual(1); - expect(lockFile).toEqual('package-lock-contents'); + expect(res.lockFile).toEqual('package-lock-contents'); }); });