From a94088ba28d9a1c5da763b6c58cdf6647576fe20 Mon Sep 17 00:00:00 2001 From: Rhys Arkins <rhys@arkins.net> Date: Tue, 1 May 2018 09:21:15 +0200 Subject: [PATCH] feat: deprecate packageFiles (#1898) This PR deprecates the use of `packageFiles` and migrates it to `includePaths` and `packageRules`. Closes #1887 --- docs/configuration.md | 10 +-- docs/design-decisions.md | 2 +- lib/config/definitions.js | 6 -- lib/config/migration.js | 28 +++++++ lib/manager/index.js | 56 +++---------- lib/manager/npm/resolve.js | 1 + test/config/__snapshots__/index.spec.js.snap | 1 - .../__snapshots__/migration.spec.js.snap | 71 ++++++++++++++++ test/config/migration.spec.js | 53 ++++++++++++ .../__snapshots__/resolve.spec.js.snap | 18 ++++- test/manager/index.spec.js | 33 -------- test/manager/resolve.spec.js | 80 ++++++------------- test/workers/branch/lock-files.spec.js | 1 + .../onboarding/pr/config-description.spec.js | 1 + .../2017-10-05-configuration-options.md | 11 --- 15 files changed, 209 insertions(+), 163 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index bcbad80802..64c3aa3dfa 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -36,10 +36,9 @@ module.exports = { repositories: [ { repository: 'singapore/repo1', - packageFiles: [ - 'package.json', + packageRules: [ { - packageFile: 'frontend/package.json', + paths: ['frontend/package.json'], labels: ['upgrade', 'frontend'], }, ], @@ -50,7 +49,7 @@ module.exports = { }, 'singapore/repo3', ], - packages: [ + packageRules: [ { packageNames: ['jquery'], labels: ['jquery', 'uhoh'], @@ -70,8 +69,7 @@ To configure any `<list>` items, separate with commas. E.g. `renovate --labels=r ### renovate.json If you add a `renovate.json` file to the root of your repository, you can use -this to override default settings. If you leave the `packageFiles` field empty -then `renovate` will still auto-discover all `package.json` files in the +this to override default settings. `renovate` will still auto-discover all `package.json` files in the repository. ### package.json diff --git a/docs/design-decisions.md b/docs/design-decisions.md index 3be0e088c8..69b2d9eb2b 100644 --- a/docs/design-decisions.md +++ b/docs/design-decisions.md @@ -43,7 +43,7 @@ are global (all repositories). Default behaviour is to auto-discover all `package.json` locations in a repository and process them all. Doing so means that "monorepos" are supported -by default. This can be overridden by the configuration option `packageFiles`, +by default. This can be overridden by the configuration option `includePaths`, where you list the file paths manually (e.g. limit to just `package.json` in root of repository). diff --git a/lib/config/definitions.js b/lib/config/definitions.js index d71dc78415..b3f9a8a385 100644 --- a/lib/config/definitions.js +++ b/lib/config/definitions.js @@ -266,12 +266,6 @@ const options = [ type: 'list', stage: 'repository', }, - { - name: 'packageFiles', - description: 'Package file paths', - type: 'list', - stage: 'branch', - }, { name: 'includePaths', description: 'Include package files only within these defined paths', diff --git a/lib/config/migration.js b/lib/config/migration.js index b5411756c4..2a384f960b 100644 --- a/lib/config/migration.js +++ b/lib/config/migration.js @@ -55,6 +55,34 @@ function migrateConfig(config) { ); } delete migratedConfig.pathRules; + } else if (key === 'packageFiles' && Array.isArray(val)) { + isMigrated = true; + const fileList = []; + for (const packageFile of val) { + if (isObject(packageFile)) { + fileList.push(packageFile.packageFile); + if (Object.keys(packageFile).length > 1) { + migratedConfig.packageRules = migratedConfig.packageRules || []; + const payload = migrateConfig(packageFile).migratedConfig; + for (const subrule of payload.packageRules || []) { + subrule.paths = [packageFile.packageFile]; + migratedConfig.packageRules.push(subrule); + } + delete payload.packageFile; + delete payload.packageRules; + if (Object.keys(payload).length) { + migratedConfig.packageRules.push({ + ...payload, + paths: [packageFile.packageFile], + }); + } + } + } else { + fileList.push(packageFile); + } + } + migratedConfig.includePaths = fileList; + delete migratedConfig.packageFiles; } else if (depTypes.includes(key)) { isMigrated = true; migratedConfig.packageRules = migratedConfig.packageRules || []; diff --git a/lib/manager/index.js b/lib/manager/index.js index c1e277dcfb..9dcbd846c3 100644 --- a/lib/manager/index.js +++ b/lib/manager/index.js @@ -24,7 +24,6 @@ for (const manager of managerList) { module.exports = { detectPackageFiles, extractDependencies, - getManager, getPackageUpdates, getUpdatedPackageFiles, resolvePackageFiles, @@ -35,19 +34,21 @@ async function detectPackageFiles(config) { logger.trace({ config }); let packageFiles = []; let fileList = await platform.getFileList(); - if (config.includePaths.length) { + if (config.includePaths && config.includePaths.length) { fileList = fileList.filter(file => config.includePaths.some( includePath => file === includePath || minimatch(file, includePath) ) ); } - fileList = fileList.filter( - file => - !config.ignorePaths.some( - ignorePath => file.includes(ignorePath) || minimatch(file, ignorePath) - ) - ); + if (config.ignorePaths && config.ignorePaths.length) { + fileList = fileList.filter( + file => + !config.ignorePaths.some( + ignorePath => file.includes(ignorePath) || minimatch(file, ignorePath) + ) + ); + } for (const manager of managerList) { logger.debug(`Detecting package files (${manager})`); const { language } = managers[manager]; @@ -167,45 +168,14 @@ async function getUpdatedPackageFiles(config) { }; } -function getManager(config, filename) { - for (const manager of managerList) { - for (const fileMatch of config[manager].fileMatch) { - if (filename.match(new RegExp(fileMatch))) { - return manager; - } - } - } - return null; -} - async function resolvePackageFiles(config) { logger.debug('manager.resolvePackageFile()'); logger.trace({ config }); - if (config.packageFiles && config.packageFiles.length) { - logger.info( - { packageFiles: config.packageFiles }, - 'Found configured packageFiles list' - ); - } - const allPackageFiles = - config.packageFiles && config.packageFiles.length - ? config.packageFiles - : await detectPackageFiles(config); + const allPackageFiles = await detectPackageFiles(config); logger.debug({ allPackageFiles }, 'allPackageFiles'); async function resolvePackageFile(p) { - let packageFile = typeof p === 'string' ? { packageFile: p } : p; - const fileName = packageFile.packageFile.split('/').pop(); - packageFile.manager = packageFile.manager || getManager(config, fileName); + let packageFile = p; const { manager } = packageFile; - if (!manager) { - // Config error - const error = new Error('config-validation'); - error.configFile = packageFile.packageFile; - error.validationError = 'Unknown file type'; - error.validationMessage = - 'Please correct the file name in your packageFiles array'; - throw error; - } if (managers[manager].resolvePackageFile) { return managers[manager].resolvePackageFile(config, packageFile); } @@ -217,10 +187,6 @@ async function resolvePackageFiles(config) { `Resolving packageFile ${JSON.stringify(packageFile.packageFile)}` ); packageFile.content = await platform.getFile(packageFile.packageFile); - if (!packageFile.content) { - logger.debug('No packageFile content'); - return null; - } return packageFile; } let queue = allPackageFiles.map(p => () => resolvePackageFile(p)); diff --git a/lib/manager/npm/resolve.js b/lib/manager/npm/resolve.js index af45443278..fe2dd95129 100644 --- a/lib/manager/npm/resolve.js +++ b/lib/manager/npm/resolve.js @@ -13,6 +13,7 @@ async function resolvePackageFile(config, inputFile) { ); const pFileRaw = await platform.getFile(packageFile.packageFile); + // istanbul ignore if if (!pFileRaw) { logger.info( { packageFile: packageFile.packageFile }, diff --git a/test/config/__snapshots__/index.spec.js.snap b/test/config/__snapshots__/index.spec.js.snap index 89c48a717c..2a1cb22b60 100644 --- a/test/config/__snapshots__/index.spec.js.snap +++ b/test/config/__snapshots__/index.spec.js.snap @@ -168,7 +168,6 @@ Object { }, "onboarding": true, "onboardingConfig": Object {}, - "packageFiles": Array [], "packageNames": Array [], "packagePatterns": Array [], "packageRules": Array [], diff --git a/test/config/__snapshots__/migration.spec.js.snap b/test/config/__snapshots__/migration.spec.js.snap index 09162a598a..93d4d03a83 100644 --- a/test/config/__snapshots__/migration.spec.js.snap +++ b/test/config/__snapshots__/migration.spec.js.snap @@ -116,6 +116,34 @@ Object { } `; +exports[`config/migration migrateConfig(config, parentConfig) it migrates more packageFiles 1`] = ` +Object { + "includePaths": Array [ + "package.json", + ], + "packageRules": Array [ + Object { + "depTypeList": Array [ + "devDependencies", + ], + "paths": Array [ + "package.json", + ], + "pinVersions": true, + }, + Object { + "depTypeList": Array [ + "dependencies", + ], + "paths": Array [ + "package.json", + ], + "pinVersions": true, + }, + ], +} +`; + exports[`config/migration migrateConfig(config, parentConfig) it migrates node to travis 1`] = ` Object { "node": Object { @@ -130,6 +158,49 @@ Object { } `; +exports[`config/migration migrateConfig(config, parentConfig) it migrates packageFiles 1`] = ` +Object { + "includePaths": Array [ + "package.json", + "backend/package.json", + "frontend/package.json", + "other/package.json", + ], + "packageRules": Array [ + Object { + "paths": Array [ + "backend/package.json", + ], + "pinVersions": false, + }, + Object { + "paths": Array [ + "frontend/package.json", + ], + "pinVersions": true, + }, + Object { + "depTypeList": Array [ + "devDependencies", + ], + "paths": Array [ + "other/package.json", + ], + "pinVersions": true, + }, + Object { + "depTypeList": Array [ + "dependencies", + ], + "paths": Array [ + "other/package.json", + ], + "pinVersions": true, + }, + ], +} +`; + exports[`config/migration migrateConfig(config, parentConfig) it migrates packages 1`] = ` Object { "packageRules": Array [ diff --git a/test/config/migration.spec.js b/test/config/migration.spec.js index 5942d9f8ee..9249b03aeb 100644 --- a/test/config/migration.spec.js +++ b/test/config/migration.spec.js @@ -261,5 +261,58 @@ describe('config/migration', () => { expect(migratedConfig.travis.enabled).toBe(true); expect(migratedConfig.node.supportPolicy).toBeDefined(); }); + it('it migrates packageFiles', () => { + const config = { + packageFiles: [ + 'package.json', + { packageFile: 'backend/package.json', pinVersions: false }, + { packageFile: 'frontend/package.json', pinVersions: true }, + { + packageFile: 'other/package.json', + devDependencies: { pinVersions: true }, + dependencies: { pinVersions: true }, + }, + ], + }; + const { isMigrated, migratedConfig } = configMigration.migrateConfig( + config, + defaultConfig + ); + expect(migratedConfig).toMatchSnapshot(); + expect(isMigrated).toBe(true); + expect(migratedConfig.includePaths).toHaveLength(4); + expect(migratedConfig.packageFiles).toBeUndefined(); + expect(migratedConfig.packageRules).toHaveLength(4); + expect(migratedConfig.packageRules[0].pinVersions).toBe(false); + expect(migratedConfig.packageRules[1].pinVersions).toBe(true); + }); + it('it migrates more packageFiles', () => { + const config = { + packageFiles: [ + { + packageFile: 'package.json', + packageRules: [ + { + pinVersions: true, + depTypeList: ['devDependencies'], + }, + { + pinVersions: true, + depTypeList: ['dependencies'], + }, + ], + }, + ], + }; + const { isMigrated, migratedConfig } = configMigration.migrateConfig( + config, + defaultConfig + ); + expect(migratedConfig).toMatchSnapshot(); + expect(isMigrated).toBe(true); + expect(migratedConfig.includePaths).toHaveLength(1); + expect(migratedConfig.packageFiles).toBeUndefined(); + expect(migratedConfig.packageRules).toHaveLength(2); + }); }); }); diff --git a/test/manager/__snapshots__/resolve.spec.js.snap b/test/manager/__snapshots__/resolve.spec.js.snap index a9694296cc..9157937dec 100644 --- a/test/manager/__snapshots__/resolve.spec.js.snap +++ b/test/manager/__snapshots__/resolve.spec.js.snap @@ -47,11 +47,12 @@ Array [ ] `; -exports[`manager/resolve resolvePackageFiles() skips if no content or no match 1`] = ` +exports[`manager/resolve resolvePackageFiles() resolves docker 1`] = ` Array [ Object { "commitMessageTopic": "{{{depName}}} Docker tag", "content": "# comment +FROM node:8 ", "digest": Object { "branchTopic": "{{{depNameSanitized}}}-{{{currentTag}}}", @@ -91,6 +92,19 @@ Array [ ] `; +exports[`manager/resolve resolvePackageFiles() resolves package files without own resolve 1`] = ` +Array [ + Object { + "content": "git_repository(\\n", + "fileMatch": Array [ + "(^|/)WORKSPACE$", + ], + "manager": "bazel", + "packageFile": "WORKSPACE", + }, +] +`; + exports[`manager/resolve resolvePackageFiles() strips npmrc with NPM_TOKEN 1`] = ` Array [ Object { @@ -107,5 +121,3 @@ Array [ }, ] `; - -exports[`manager/resolve resolvePackageFiles() uses packageFiles if already configured and raises error if not found 1`] = `Array []`; diff --git a/test/manager/index.spec.js b/test/manager/index.spec.js index f8af1dcc1d..9f2b3f56c8 100644 --- a/test/manager/index.spec.js +++ b/test/manager/index.spec.js @@ -136,39 +136,6 @@ describe('manager', () => { expect(res).toHaveLength(1); }); }); - describe('getManager', () => { - it('rejects unknown files', () => { - expect(manager.getManager(defaultConfig, 'WORKSPACER')).toBe(null); - }); - it('detects files in root', () => { - expect(manager.getManager(defaultConfig, 'WORKSPACE')).toBe('bazel'); - expect(manager.getManager(defaultConfig, 'Dockerfile')).toBe('docker'); - expect(manager.getManager(defaultConfig, 'package.js')).toBe('meteor'); - expect(manager.getManager(defaultConfig, 'package.json')).toBe('npm'); - expect(manager.getManager(defaultConfig, '.nvmrc')).toBe('nvm'); - expect(manager.getManager(defaultConfig, '.travis.yml')).toBe('travis'); - }); - it('detects nested files', () => { - expect(manager.getManager(defaultConfig, 'foo/bar/WORKSPACE')).toBe( - 'bazel' - ); - expect(manager.getManager(defaultConfig, 'backend/Dockerfile')).toBe( - 'docker' - ); - expect(manager.getManager(defaultConfig, 'package/a/package.js')).toBe( - 'meteor' - ); - expect(manager.getManager(defaultConfig, 'frontend/package.json')).toBe( - 'npm' - ); - expect(manager.getManager(defaultConfig, 'subfolder-1/.nvmrc')).toBe( - null - ); - expect(manager.getManager(defaultConfig, 'subfolder-2/.travis.yml')).toBe( - null - ); - }); - }); describe('getUpdatedPackageFiles', () => { let config; beforeEach(() => { diff --git a/test/manager/resolve.spec.js b/test/manager/resolve.spec.js index bbc5a53a62..38637091a2 100644 --- a/test/manager/resolve.spec.js +++ b/test/manager/resolve.spec.js @@ -13,40 +13,24 @@ beforeEach(() => { describe('manager/resolve', () => { describe('resolvePackageFiles()', () => { - it('handles wrong filenames', async () => { - config.packageFiles = ['wrong.txt']; - let e; - try { - await resolvePackageFiles(config); - } catch (err) { - e = err; - } - expect(e).toBeDefined(); - }); - it('uses packageFiles if already configured and raises error if not found', async () => { - config.packageFiles = [ - 'package.json', - { packageFile: 'backend/package.json' }, - ]; - const res = await resolvePackageFiles(config); - expect(res.packageFiles).toMatchSnapshot(); - expect(res.errors).toHaveLength(2); + beforeEach(() => { + manager.detectPackageFiles = jest.fn(); }); it('detect package.json and adds error if cannot parse (onboarding)', async () => { - manager.detectPackageFiles = jest.fn(() => [ + manager.detectPackageFiles.mockReturnValueOnce([ { packageFile: 'package.json', manager: 'npm' }, ]); - platform.getFileList.mockReturnValue(['package.json']); + platform.getFileList.mockReturnValueOnce(['package.json']); platform.getFile.mockReturnValueOnce('not json'); const res = await resolvePackageFiles(config); expect(res.packageFiles).toMatchSnapshot(); expect(res.errors).toHaveLength(1); }); it('detect package.json and throws error if cannot parse (onboarded)', async () => { - manager.detectPackageFiles = jest.fn(() => [ + manager.detectPackageFiles.mockReturnValueOnce([ { packageFile: 'package.json', manager: 'npm' }, ]); - platform.getFileList.mockReturnValue(['package.json']); + platform.getFileList.mockReturnValueOnce(['package.json']); platform.getFile.mockReturnValueOnce('not json'); config.repoIsOnboarded = true; let e; @@ -59,7 +43,7 @@ describe('manager/resolve', () => { expect(e).toMatchSnapshot(); }); it('clears npmrc and yarnrc fields', async () => { - manager.detectPackageFiles = jest.fn(() => [ + manager.detectPackageFiles.mockReturnValueOnce([ { packageFile: 'package.json', manager: 'npm' }, ]); const pJson = { @@ -70,13 +54,14 @@ describe('manager/resolve', () => { }, }; platform.getFile.mockReturnValueOnce(JSON.stringify(pJson)); - platform.getFileList.mockReturnValue(['package.json']); + platform.getFileList.mockReturnValueOnce(['package.json']); + platform.getFileList.mockReturnValueOnce(['package.json']); const res = await resolvePackageFiles(config); expect(res.packageFiles).toMatchSnapshot(); expect(res.warnings).toHaveLength(0); }); it('detects accompanying files', async () => { - manager.detectPackageFiles = jest.fn(() => [ + manager.detectPackageFiles.mockReturnValueOnce([ { packageFile: 'package.json', manager: 'npm' }, ]); platform.getFileList.mockReturnValue([ @@ -95,43 +80,24 @@ describe('manager/resolve', () => { expect(res.packageFiles).toMatchSnapshot(); expect(res.warnings).toHaveLength(0); }); - it('detects meteor and docker and travis and bazel and nvm', async () => { - config.packageFiles = [ - 'package.js', - { packageFile: '.circleci/config.yml', manager: 'circleci' }, - 'Dockerfile', - 'docker-compose.yml', - '.travis.yml', - 'WORKSPACE', - '.nvmrc', - ]; - platform.getFile.mockReturnValueOnce('{}'); // package.js - platform.getFile.mockReturnValueOnce(' - image: node:8\n'); // CircleCI - platform.getFile.mockReturnValueOnce('# comment\nFROM node:8\n'); // Dockerfile - platform.getFile.mockReturnValueOnce('image: node:8\n'); // Docker Compose - platform.getFile.mockReturnValueOnce('# travis'); // .travis.yml - platform.getFile.mockReturnValueOnce('# WORKSPACE'); // Dockerfileyarn j - platform.getFile.mockReturnValueOnce('8.9\n'); // Dockerfile + it('resolves docker', async () => { + platform.getFileList.mockReturnValue(['Dockerfile']); + platform.getFile.mockReturnValue('# comment\nFROM node:8\n'); // Dockerfile const res = await resolvePackageFiles(config); - expect(res.packageFiles).toHaveLength(7); + expect(res.packageFiles).toMatchSnapshot(); + expect(res.packageFiles).toHaveLength(1); + expect(res.warnings).toHaveLength(0); }); - it('skips if no content or no match', async () => { - config.packageFiles = [ - 'Dockerfile', - 'other/Dockerfile', - 'docker-compose.yml', - '.travis.yml', - { packageFile: '.circleci/config.yml', manager: 'circleci' }, - 'WORKSPACE', - 'package.js', - '.nvmrc', - ]; - platform.getFile.mockReturnValueOnce('# comment\n'); // Dockerfile + it('resolves package files without own resolve', async () => { + platform.getFileList.mockReturnValue(['WORKSPACE']); + platform.getFile.mockReturnValue('git_repository(\n'); // WORKSPACE const res = await resolvePackageFiles(config); expect(res.packageFiles).toMatchSnapshot(); + expect(res.packageFiles).toHaveLength(1); + expect(res.warnings).toHaveLength(0); }); it('strips npmrc with NPM_TOKEN', async () => { - manager.detectPackageFiles = jest.fn(() => [ + manager.detectPackageFiles.mockReturnValueOnce([ { packageFile: 'package.json', manager: 'npm' }, ]); platform.getFileList.mockReturnValue(['package.json', '.npmrc']); @@ -146,7 +112,7 @@ describe('manager/resolve', () => { expect(res.warnings).toHaveLength(0); }); it('checks if renovate config in nested package.json throws an error', async () => { - manager.detectPackageFiles = jest.fn(() => [ + manager.detectPackageFiles.mockReturnValueOnce([ { packageFile: 'package.json', manager: 'npm' }, ]); platform.getFileList.mockReturnValue(['test/package.json']); diff --git a/test/workers/branch/lock-files.spec.js b/test/workers/branch/lock-files.spec.js index e92e45b5aa..6682a22771 100644 --- a/test/workers/branch/lock-files.spec.js +++ b/test/workers/branch/lock-files.spec.js @@ -544,6 +544,7 @@ describe('workers/branch/lock-files', () => { shrinkwrapYamlDirs: [], lernaDirs: ['.'], }); + config.packageFiles = []; config.lernaLockFile = 'npm'; lerna.generateLockFiles.mockReturnValueOnce({ error: false }); const res = await getUpdatedLockFiles(config); diff --git a/test/workers/repository/onboarding/pr/config-description.spec.js b/test/workers/repository/onboarding/pr/config-description.spec.js index c753a8d28d..67738764ca 100644 --- a/test/workers/repository/onboarding/pr/config-description.spec.js +++ b/test/workers/repository/onboarding/pr/config-description.spec.js @@ -46,6 +46,7 @@ describe('workers/repository/onboarding/pr/config-description', () => { expect(res.indexOf('Docker-only')).toBe(-1); }); it('assignees, labels and schedule', () => { + config.packageFiles = []; config.assignees = ['someone', '@someone-else']; config.labels = ['renovate', 'deps']; config.schedule = ['before 5am']; diff --git a/website/docs/_posts/2017-10-05-configuration-options.md b/website/docs/_posts/2017-10-05-configuration-options.md index f8629d6681..621cd806f6 100644 --- a/website/docs/_posts/2017-10-05-configuration-options.md +++ b/website/docs/_posts/2017-10-05-configuration-options.md @@ -724,17 +724,6 @@ Configuration specific for `.nvmrc` files. For settings common to all node.js version updates (e.g. travis, nvm, etc) you can use the `node` object instead. -## packageFiles - -A manually provisioned list of package files to use. - -| name | value | -| ------- | ---------------- | -| type | array of strings | -| default | `[]` | - -If left default then package file autodiscovery will be used, so only change this setting if you wish to manually specify a limited set of `package.json` or other package files to renovate. - ## packageNames A list of package names inside a package rule. -- GitLab