From 31e7c8a881b36045d3595180ef40b004f5fc74dd Mon Sep 17 00:00:00 2001 From: Rhys Arkins <rhys@keylocation.sg> Date: Wed, 25 Oct 2017 06:48:08 +0200 Subject: [PATCH] refactor: move languages from branch worker to manager (#1044) --- .../docker/update.js} | 0 lib/manager/index.js | 76 ++++++++++++++++++ .../meteor/update.js} | 0 .../package-json.js => manager/npm/update.js} | 0 lib/workers/branch/index.js | 2 +- lib/workers/branch/package-files.js | 78 ------------------- .../docker/__snapshots__/update.spec.js.snap} | 0 .../docker/update.spec.js} | 2 +- test/manager/index.spec.js | 63 +++++++++++++++ .../meteor/__snapshots__/update.spec.js.snap} | 0 .../meteor/update.spec.js} | 8 +- .../npm/update.spec.js} | 12 +-- test/workers/branch/index.spec.js | 20 ++--- test/workers/branch/package-files.spec.js | 68 ---------------- 14 files changed, 161 insertions(+), 168 deletions(-) rename lib/{workers/branch/dockerfile.js => manager/docker/update.js} (100%) rename lib/{workers/branch/package-js.js => manager/meteor/update.js} (100%) rename lib/{workers/branch/package-json.js => manager/npm/update.js} (100%) delete mode 100644 lib/workers/branch/package-files.js rename test/{workers/branch/__snapshots__/dockerfile.spec.js.snap => manager/docker/__snapshots__/update.spec.js.snap} (100%) rename test/{workers/branch/dockerfile.spec.js => manager/docker/update.spec.js} (93%) rename test/{workers/branch/__snapshots__/package-js.spec.js.snap => manager/meteor/__snapshots__/update.spec.js.snap} (100%) rename test/{workers/branch/package-js.spec.js => manager/meteor/update.spec.js} (83%) rename test/{workers/branch/package-json.spec.js => manager/npm/update.spec.js} (84%) delete mode 100644 test/workers/branch/package-files.spec.js diff --git a/lib/workers/branch/dockerfile.js b/lib/manager/docker/update.js similarity index 100% rename from lib/workers/branch/dockerfile.js rename to lib/manager/docker/update.js diff --git a/lib/manager/index.js b/lib/manager/index.js index 746735b524..eecfc92e59 100644 --- a/lib/manager/index.js +++ b/lib/manager/index.js @@ -5,9 +5,14 @@ const dockerDetect = require('./docker/detect'); const meteorDetect = require('./meteor/detect'); const npmDetect = require('./npm/detect'); +const npmUpdater = require('./npm/update'); +const meteorUpdater = require('./meteor/update'); +const dockerfileHelper = require('./docker/update'); + module.exports = { detectPackageFiles, getPackageUpdates, + getUpdatedPackageFiles, }; async function detectPackageFiles(input) { @@ -50,3 +55,74 @@ async function getPackageUpdates(config) { config.logger.info(`Cannot find manager for ${config.packageFile}`); throw new Error('Unsupported package manager'); } + +async function getUpdatedPackageFiles(config) { + const { logger } = config; + const updatedPackageFiles = {}; + + for (const upgrade of config.upgrades) { + if (upgrade.type !== 'lockFileMaintenance') { + const existingContent = + updatedPackageFiles[upgrade.packageFile] || + (await config.api.getFileContent( + upgrade.packageFile, + config.parentBranch + )); + let newContent = existingContent; + if (upgrade.packageFile.endsWith('package.json')) { + newContent = npmUpdater.setNewValue( + existingContent, + upgrade.depType, + upgrade.depName, + upgrade.newVersion, + config.logger + ); + } else if (upgrade.packageFile.endsWith('package.js')) { + newContent = meteorUpdater.setNewValue( + existingContent, + upgrade.depName, + upgrade.currentVersion, + upgrade.newVersion, + config.logger + ); + } else if (upgrade.packageFile.endsWith('Dockerfile')) { + newContent = dockerfileHelper.setNewValue( + existingContent, + upgrade.depName, + upgrade.currentFrom, + upgrade.newFrom, + config.logger + ); + } + if (!newContent) { + if (config.parentBranch && config.canRebase) { + logger.info('Rebasing branch after error updating content'); + return getUpdatedPackageFiles({ + ...config, + parentBranch: undefined, + }); + } + throw new Error('Error updating branch content and cannot rebase'); + } + if (newContent !== existingContent) { + if (config.parentBranch && config.canRebase) { + // This ensure it's always 1 commit from Renovate + logger.info('Need to update package file so will rebase first'); + return getUpdatedPackageFiles({ + ...config, + parentBranch: undefined, + }); + } + logger.debug('Updating packageFile content'); + updatedPackageFiles[upgrade.packageFile] = newContent; + } + } + } + return { + parentBranch: config.parentBranch, // Need to overwrite original config + updatedPackageFiles: Object.keys(updatedPackageFiles).map(packageFile => ({ + name: packageFile, + contents: updatedPackageFiles[packageFile], + })), + }; +} diff --git a/lib/workers/branch/package-js.js b/lib/manager/meteor/update.js similarity index 100% rename from lib/workers/branch/package-js.js rename to lib/manager/meteor/update.js diff --git a/lib/workers/branch/package-json.js b/lib/manager/npm/update.js similarity index 100% rename from lib/workers/branch/package-json.js rename to lib/manager/npm/update.js diff --git a/lib/workers/branch/index.js b/lib/workers/branch/index.js index 5a919cfd41..52c2192f5e 100644 --- a/lib/workers/branch/index.js +++ b/lib/workers/branch/index.js @@ -1,5 +1,5 @@ const schedule = require('./schedule'); -const { getUpdatedPackageFiles } = require('./package-files'); +const { getUpdatedPackageFiles } = require('../../manager'); const { getUpdatedLockFiles } = require('./lock-files'); const { commitFilesToBranch } = require('./commit'); const { getParentBranch } = require('./parent'); diff --git a/lib/workers/branch/package-files.js b/lib/workers/branch/package-files.js deleted file mode 100644 index eaa3f305e1..0000000000 --- a/lib/workers/branch/package-files.js +++ /dev/null @@ -1,78 +0,0 @@ -const packageJsonHelper = require('./package-json'); -const packageJsHelper = require('./package-js'); -const dockerfileHelper = require('./dockerfile'); - -module.exports = { - getUpdatedPackageFiles, -}; - -async function getUpdatedPackageFiles(config) { - const { logger } = config; - const updatedPackageFiles = {}; - - for (const upgrade of config.upgrades) { - if (upgrade.type !== 'lockFileMaintenance') { - const existingContent = - updatedPackageFiles[upgrade.packageFile] || - (await config.api.getFileContent( - upgrade.packageFile, - config.parentBranch - )); - let newContent = existingContent; - if (upgrade.packageFile.endsWith('package.json')) { - newContent = packageJsonHelper.setNewValue( - existingContent, - upgrade.depType, - upgrade.depName, - upgrade.newVersion, - config.logger - ); - } else if (upgrade.packageFile.endsWith('package.js')) { - newContent = packageJsHelper.setNewValue( - existingContent, - upgrade.depName, - upgrade.currentVersion, - upgrade.newVersion, - config.logger - ); - } else if (upgrade.packageFile.endsWith('Dockerfile')) { - newContent = dockerfileHelper.setNewValue( - existingContent, - upgrade.depName, - upgrade.currentFrom, - upgrade.newFrom, - config.logger - ); - } - if (!newContent) { - if (config.parentBranch && config.canRebase) { - logger.info('Rebasing branch after error updating content'); - return getUpdatedPackageFiles({ - ...config, - parentBranch: undefined, - }); - } - throw new Error('Error updating branch content and cannot rebase'); - } - if (newContent !== existingContent) { - if (config.parentBranch && config.canRebase) { - // This ensure it's always 1 commit from Renovate - logger.info('Need to update package file so will rebase first'); - return getUpdatedPackageFiles({ - ...config, - parentBranch: undefined, - }); - } - logger.debug('Updating packageFile content'); - updatedPackageFiles[upgrade.packageFile] = newContent; - } - } - } - return { - parentBranch: config.parentBranch, // Need to overwrite original config - updatedPackageFiles: Object.keys(updatedPackageFiles).map(packageFile => ({ - name: packageFile, - contents: updatedPackageFiles[packageFile], - })), - }; -} diff --git a/test/workers/branch/__snapshots__/dockerfile.spec.js.snap b/test/manager/docker/__snapshots__/update.spec.js.snap similarity index 100% rename from test/workers/branch/__snapshots__/dockerfile.spec.js.snap rename to test/manager/docker/__snapshots__/update.spec.js.snap diff --git a/test/workers/branch/dockerfile.spec.js b/test/manager/docker/update.spec.js similarity index 93% rename from test/workers/branch/dockerfile.spec.js rename to test/manager/docker/update.spec.js index f1dd4765c6..4ba3b2f7bf 100644 --- a/test/workers/branch/dockerfile.spec.js +++ b/test/manager/docker/update.spec.js @@ -1,4 +1,4 @@ -const dockerfile = require('../../../lib/workers/branch/dockerfile'); +const dockerfile = require('../../../lib/manager/docker/update'); const logger = require('../../_fixtures/logger'); describe('workers/branch/dockerfile', () => { diff --git a/test/manager/index.spec.js b/test/manager/index.spec.js index 94d1ed5830..87b5bb8eab 100644 --- a/test/manager/index.spec.js +++ b/test/manager/index.spec.js @@ -2,6 +2,12 @@ const logger = require('../_fixtures/logger'); const defaultConfig = require('../../lib/config/defaults').getConfig(); const manager = require('../../lib/manager'); +const npmUpdater = require('../../lib/manager/npm/update'); +const meteorUpdater = require('../../lib/manager/meteor/update'); +const dockerUpdater = require('../../lib/manager/docker/update'); + +const { getUpdatedPackageFiles } = manager; + describe('manager', () => { describe('detectPackageFiles(config)', () => { let config; @@ -72,4 +78,61 @@ describe('manager', () => { expect(res.warnings).toMatchSnapshot(); }); }); + describe('getUpdatedPackageFiles', () => { + let config; + beforeEach(() => { + config = { + ...defaultConfig, + api: { getFileContent: jest.fn() }, + logger, + parentBranch: 'some-branch', + }; + npmUpdater.setNewValue = jest.fn(); + dockerUpdater.setNewValue = jest.fn(); + meteorUpdater.setNewValue = jest.fn(); + }); + it('returns empty if lock file maintenance', async () => { + config.upgrades = [{ type: 'lockFileMaintenance' }]; + const res = await getUpdatedPackageFiles(config); + expect(res.updatedPackageFiles).toHaveLength(0); + }); + it('recurses if setNewValue error', async () => { + config.parentBranch = 'some-branch'; + config.canRebase = true; + config.upgrades = [{ packageFile: 'package.json' }]; + npmUpdater.setNewValue.mockReturnValueOnce(null); + npmUpdater.setNewValue.mockReturnValueOnce('some content'); + const res = await getUpdatedPackageFiles(config); + expect(res.updatedPackageFiles).toHaveLength(1); + }); + it('errors if cannot rebase', async () => { + config.upgrades = [{ packageFile: 'package.json' }]; + let e; + try { + await getUpdatedPackageFiles(config); + } catch (err) { + e = err; + } + expect(e).toBeDefined(); + }); + it('returns updated files', async () => { + config.parentBranch = 'some-branch'; + config.canRebase = true; + config.upgrades = [ + { packageFile: 'package.json' }, + { packageFile: 'Dockerfile' }, + { packageFile: 'packages/foo/package.js' }, + ]; + config.api.getFileContent.mockReturnValueOnce('old content 1'); + config.api.getFileContent.mockReturnValueOnce('old content 1'); + config.api.getFileContent.mockReturnValueOnce('old content 2'); + config.api.getFileContent.mockReturnValueOnce('old content 3'); + npmUpdater.setNewValue.mockReturnValueOnce('new content 1'); + npmUpdater.setNewValue.mockReturnValueOnce('new content 1+'); + dockerUpdater.setNewValue.mockReturnValueOnce('new content 2'); + meteorUpdater.setNewValue.mockReturnValueOnce('old content 3'); + const res = await getUpdatedPackageFiles(config); + expect(res.updatedPackageFiles).toHaveLength(2); + }); + }); }); diff --git a/test/workers/branch/__snapshots__/package-js.spec.js.snap b/test/manager/meteor/__snapshots__/update.spec.js.snap similarity index 100% rename from test/workers/branch/__snapshots__/package-js.spec.js.snap rename to test/manager/meteor/__snapshots__/update.spec.js.snap diff --git a/test/workers/branch/package-js.spec.js b/test/manager/meteor/update.spec.js similarity index 83% rename from test/workers/branch/package-js.spec.js rename to test/manager/meteor/update.spec.js index 3dc3ddf421..40ecd679fb 100644 --- a/test/workers/branch/package-js.spec.js +++ b/test/manager/meteor/update.spec.js @@ -1,6 +1,6 @@ const fs = require('fs'); const path = require('path'); -const packageJs = require('../../../lib/workers/branch/package-js'); +const meteorUpdater = require('../../../lib/manager/meteor/update'); const logger = require('../../_fixtures/logger'); function readFixture(fixture) { @@ -16,7 +16,7 @@ const input02Content = readFixture('package-2.js'); describe('workers/branch/package-js', () => { describe('.setNewValue(currentFileContent, depName, currentVersion, newVersion, logger)', () => { it('replaces a dependency value', () => { - const testContent = packageJs.setNewValue( + const testContent = meteorUpdater.setNewValue( input01Content, 'xmldom', '0.1.19', @@ -26,7 +26,7 @@ describe('workers/branch/package-js', () => { expect(testContent).toMatchSnapshot(); }); it('handles alternative quotes and white space', () => { - const testContent = packageJs.setNewValue( + const testContent = meteorUpdater.setNewValue( input02Content, 'xmldom', '0.1.19', @@ -36,7 +36,7 @@ describe('workers/branch/package-js', () => { expect(testContent).toMatchSnapshot(); }); it('handles the case where the desired version is already supported', () => { - const testContent = packageJs.setNewValue( + const testContent = meteorUpdater.setNewValue( input01Content, 'query-string', '0.2.0', diff --git a/test/workers/branch/package-json.spec.js b/test/manager/npm/update.spec.js similarity index 84% rename from test/workers/branch/package-json.spec.js rename to test/manager/npm/update.spec.js index 34532df44d..d926334e1e 100644 --- a/test/workers/branch/package-json.spec.js +++ b/test/manager/npm/update.spec.js @@ -1,6 +1,6 @@ const fs = require('fs'); const path = require('path'); -const packageJson = require('../../../lib/workers/branch/package-json'); +const npmUpdater = require('../../../lib/manager/npm/update'); const logger = require('../../_fixtures/logger'); function readFixture(fixture) { @@ -16,7 +16,7 @@ describe('workers/branch/package-json', () => { describe('.setNewValue(currentFileContent, depType, depName, newVersion, logger)', () => { it('replaces a dependency value', () => { const outputContent = readFixture('outputs/011.json'); - const testContent = packageJson.setNewValue( + const testContent = npmUpdater.setNewValue( input01Content, 'dependencies', 'cheerio', @@ -27,7 +27,7 @@ describe('workers/branch/package-json', () => { }); it('replaces only the first instance of a value', () => { const outputContent = readFixture('outputs/012.json'); - const testContent = packageJson.setNewValue( + const testContent = npmUpdater.setNewValue( input01Content, 'devDependencies', 'angular-touch', @@ -38,7 +38,7 @@ describe('workers/branch/package-json', () => { }); it('replaces only the second instance of a value', () => { const outputContent = readFixture('outputs/013.json'); - const testContent = packageJson.setNewValue( + const testContent = npmUpdater.setNewValue( input01Content, 'devDependencies', 'angular-sanitize', @@ -48,7 +48,7 @@ describe('workers/branch/package-json', () => { testContent.should.equal(outputContent); }); it('handles the case where the desired version is already supported', () => { - const testContent = packageJson.setNewValue( + const testContent = npmUpdater.setNewValue( input01Content, 'devDependencies', 'angular-touch', @@ -58,7 +58,7 @@ describe('workers/branch/package-json', () => { testContent.should.equal(input01Content); }); it('returns null if throws error', () => { - const testContent = packageJson.setNewValue( + const testContent = npmUpdater.setNewValue( input01Content, 'blah', 'angular-touch-not', diff --git a/test/workers/branch/index.spec.js b/test/workers/branch/index.spec.js index 785f4d3a52..326b839e7e 100644 --- a/test/workers/branch/index.spec.js +++ b/test/workers/branch/index.spec.js @@ -4,17 +4,17 @@ const defaultConfig = require('../../../lib/config/defaults').getConfig(); const schedule = require('../../../lib/workers/branch/schedule'); const checkExisting = require('../../../lib/workers/branch/check-existing'); const parent = require('../../../lib/workers/branch/parent'); -const packageFiles = require('../../../lib/workers/branch/package-files'); +const manager = require('../../../lib/manager'); const lockFiles = require('../../../lib/workers/branch/lock-files'); const commit = require('../../../lib/workers/branch/commit'); const statusChecks = require('../../../lib/workers/branch/status-checks'); const automerge = require('../../../lib/workers/branch/automerge'); const prWorker = require('../../../lib/workers/pr'); +jest.mock('../../../lib/manager'); jest.mock('../../../lib/workers/branch/schedule'); jest.mock('../../../lib/workers/branch/check-existing'); jest.mock('../../../lib/workers/branch/parent'); -jest.mock('../../../lib/workers/branch/package-files'); jest.mock('../../../lib/workers/branch/lock-files'); jest.mock('../../../lib/workers/branch/commit'); jest.mock('../../../lib/workers/branch/status-checks'); @@ -82,7 +82,7 @@ describe('workers/branch', () => { expect(config.logger.error.mock.calls).toHaveLength(0); }); it('returns if no branch exists', async () => { - packageFiles.getUpdatedPackageFiles.mockReturnValueOnce({ + manager.getUpdatedPackageFiles.mockReturnValueOnce({ updatedPackageFiles: [], }); lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ @@ -94,7 +94,7 @@ describe('workers/branch', () => { expect(commit.commitFilesToBranch.mock.calls).toHaveLength(1); }); it('returns if branch automerged', async () => { - packageFiles.getUpdatedPackageFiles.mockReturnValueOnce({ + manager.getUpdatedPackageFiles.mockReturnValueOnce({ updatedPackageFiles: [{}], }); lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ @@ -109,7 +109,7 @@ describe('workers/branch', () => { expect(prWorker.ensurePr.mock.calls).toHaveLength(0); }); it('ensures PR and tries automerge', async () => { - packageFiles.getUpdatedPackageFiles.mockReturnValueOnce({ + manager.getUpdatedPackageFiles.mockReturnValueOnce({ updatedPackageFiles: [{}], }); lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ @@ -126,7 +126,7 @@ describe('workers/branch', () => { expect(prWorker.checkAutoMerge.mock.calls).toHaveLength(1); }); it('ensures PR and adds lock file error comment', async () => { - packageFiles.getUpdatedPackageFiles.mockReturnValueOnce({ + manager.getUpdatedPackageFiles.mockReturnValueOnce({ updatedPackageFiles: [{}], }); lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ @@ -145,7 +145,7 @@ describe('workers/branch', () => { expect(prWorker.checkAutoMerge.mock.calls).toHaveLength(0); }); it('ensures PR and adds lock file error comment recreate closed', async () => { - packageFiles.getUpdatedPackageFiles.mockReturnValueOnce({ + manager.getUpdatedPackageFiles.mockReturnValueOnce({ updatedPackageFiles: [{}], }); lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ @@ -165,13 +165,13 @@ describe('workers/branch', () => { expect(prWorker.checkAutoMerge.mock.calls).toHaveLength(0); }); it('swallows branch errors', async () => { - packageFiles.getUpdatedPackageFiles.mockImplementationOnce(() => { + manager.getUpdatedPackageFiles.mockImplementationOnce(() => { throw new Error('some error'); }); await branchWorker.processBranch(config); }); it('throws and swallows branch errors', async () => { - packageFiles.getUpdatedPackageFiles.mockReturnValueOnce({ + manager.getUpdatedPackageFiles.mockReturnValueOnce({ updatedPackageFiles: [{}], }); lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ @@ -181,7 +181,7 @@ describe('workers/branch', () => { await branchWorker.processBranch(config); }); it('swallows pr errors', async () => { - packageFiles.getUpdatedPackageFiles.mockReturnValueOnce({ + manager.getUpdatedPackageFiles.mockReturnValueOnce({ updatedPackageFiles: [{}], }); lockFiles.getUpdatedLockFiles.mockReturnValueOnce({ diff --git a/test/workers/branch/package-files.spec.js b/test/workers/branch/package-files.spec.js deleted file mode 100644 index d667d43ba4..0000000000 --- a/test/workers/branch/package-files.spec.js +++ /dev/null @@ -1,68 +0,0 @@ -const packageJsonHelper = require('../../../lib/workers/branch/package-json'); -const packageJsHelper = require('../../../lib/workers/branch/package-js'); -const dockerHelper = require('../../../lib/workers/branch/dockerfile'); -const { - getUpdatedPackageFiles, -} = require('../../../lib/workers/branch/package-files'); -const defaultConfig = require('../../../lib/config/defaults').getConfig(); -const logger = require('../../_fixtures/logger'); - -describe('workers/branch/package-files', () => { - describe('getUpdatedPackageFiles', () => { - let config; - beforeEach(() => { - config = { - ...defaultConfig, - api: { getFileContent: jest.fn() }, - logger, - parentBranch: 'some-branch', - }; - packageJsonHelper.setNewValue = jest.fn(); - dockerHelper.setNewValue = jest.fn(); - packageJsHelper.setNewValue = jest.fn(); - }); - it('returns empty if lock file maintenance', async () => { - config.upgrades = [{ type: 'lockFileMaintenance' }]; - const res = await getUpdatedPackageFiles(config); - expect(res.updatedPackageFiles).toHaveLength(0); - }); - it('recurses if setNewValue error', async () => { - config.parentBranch = 'some-branch'; - config.canRebase = true; - config.upgrades = [{ packageFile: 'package.json' }]; - packageJsonHelper.setNewValue.mockReturnValueOnce(null); - packageJsonHelper.setNewValue.mockReturnValueOnce('some content'); - const res = await getUpdatedPackageFiles(config); - expect(res.updatedPackageFiles).toHaveLength(1); - }); - it('errors if cannot rebase', async () => { - config.upgrades = [{ packageFile: 'package.json' }]; - let e; - try { - await getUpdatedPackageFiles(config); - } catch (err) { - e = err; - } - expect(e).toBeDefined(); - }); - it('returns updated files', async () => { - config.parentBranch = 'some-branch'; - config.canRebase = true; - config.upgrades = [ - { packageFile: 'package.json' }, - { packageFile: 'Dockerfile' }, - { packageFile: 'packages/foo/package.js' }, - ]; - config.api.getFileContent.mockReturnValueOnce('old content 1'); - config.api.getFileContent.mockReturnValueOnce('old content 1'); - config.api.getFileContent.mockReturnValueOnce('old content 2'); - config.api.getFileContent.mockReturnValueOnce('old content 3'); - packageJsonHelper.setNewValue.mockReturnValueOnce('new content 1'); - packageJsonHelper.setNewValue.mockReturnValueOnce('new content 1+'); - dockerHelper.setNewValue.mockReturnValueOnce('new content 2'); - packageJsHelper.setNewValue.mockReturnValueOnce('old content 3'); - const res = await getUpdatedPackageFiles(config); - expect(res.updatedPackageFiles).toHaveLength(2); - }); - }); -}); -- GitLab