From 7d493a14bf2772351639f16b008bc0bee35602eb Mon Sep 17 00:00:00 2001
From: Rhys Arkins <rhys@keylocation.sg>
Date: Fri, 28 Jul 2017 21:15:27 +0200
Subject: [PATCH] feat: Log warnings when unknown configuration options or
option types found (#554)
This PR adds detection and log warnings for the following config validation errors:
- Unknown config option (e.g misspelling of a valid config option)
- Config option is a wrong type (e.g. string instead of boolean)
It does *not* propagate this warning to the user (i.e. in onboarding or PRs) yet due to the high chance that we'll find a false negative. I will watch logs for a week or so and then once happy with results will activate user-visible warnings via #556.
Closes #548, Closes #555
---
lib/config/validation.js | 84 +++++++++++++++++++
lib/workers/global/index.js | 5 ++
lib/workers/package-file/index.js | 22 ++++-
lib/workers/repository/apis.js | 11 +++
.../__snapshots__/validation.spec.js.snap | 39 +++++++++
test/config/validation.spec.js | 30 +++++++
test/workers/global/index.spec.js | 9 +-
.../__snapshots__/index.spec.js.snap | 3 +
test/workers/package-file/index.spec.js | 6 ++
.../__snapshots__/apis.spec.js.snap | 6 +-
test/workers/repository/apis.spec.js | 26 ++++--
11 files changed, 229 insertions(+), 12 deletions(-)
create mode 100644 lib/config/validation.js
create mode 100644 test/config/__snapshots__/validation.spec.js.snap
create mode 100644 test/config/validation.spec.js
create mode 100644 test/workers/package-file/__snapshots__/index.spec.js.snap
diff --git a/lib/config/validation.js b/lib/config/validation.js
new file mode 100644
index 0000000000..7902f36f52
--- /dev/null
+++ b/lib/config/validation.js
@@ -0,0 +1,84 @@
+const options = require('./definitions').getOptions();
+
+const optionTypes = {};
+options.forEach(option => {
+ optionTypes[option.name] = [option.type];
+});
+
+module.exports = {
+ validateConfig,
+};
+
+function validateConfig(config) {
+ let errors = [];
+
+ function isIgnored(key) {
+ const ignoredNodes = ['api', 'depType'];
+ return ignoredNodes.indexOf(key) !== -1;
+ }
+
+ function isAFunction(value) {
+ const getType = {};
+ return value && getType.toString.call(value) === '[object Function]';
+ }
+
+ function isObject(obj) {
+ return Object.prototype.toString.call(obj) === '[object Object]';
+ }
+
+ for (const key of Object.keys(config)) {
+ const val = config[key];
+ if (
+ !isIgnored(key) && // We need to ignore some reserved keys
+ !isAFunction(val) // Ignore all functions
+ ) {
+ if (!optionTypes[key]) {
+ errors.push({
+ depName: 'Configuration Error',
+ message: `Invalid configuration option: \`${key}\``,
+ });
+ } else {
+ const type = optionTypes[key].toString();
+ if (type === 'boolean') {
+ if (val !== true && val !== false) {
+ errors.push({
+ depName: 'Configuration Error',
+ message: `Configuration option \`${key}\` should be boolean`,
+ });
+ }
+ } else if (type === 'list') {
+ if (!Array.isArray(val)) {
+ errors.push({
+ depName: 'Configuration Error',
+ message: `Configuration option \`${key}\` should be a list (Array)`,
+ });
+ }
+ } else if (type === 'string') {
+ if (!(typeof val === 'string' || val instanceof String)) {
+ errors.push({
+ depName: 'Configuration Error',
+ message: `Configuration option \`${key}\` should be a string`,
+ });
+ }
+ } else if (type === 'integer') {
+ if (val !== parseInt(val, 10)) {
+ errors.push({
+ depName: 'Configuration Error',
+ message: `Configuration option \`${key}\` should be an integer`,
+ });
+ }
+ } else if (type === 'json') {
+ if (isObject(val)) {
+ errors = errors.concat(module.exports.validateConfig(val));
+ } else {
+ errors.push({
+ depName: 'Configuration Error',
+ message: `Configuration option \`${key}\` should be a json object`,
+ });
+ }
+ }
+ }
+ }
+ }
+ return errors;
+}
diff --git a/lib/workers/global/index.js b/lib/workers/global/index.js
index f87a55d244..9118166c5b 100644
--- a/lib/workers/global/index.js
+++ b/lib/workers/global/index.js
@@ -2,6 +2,7 @@ const logger = require('../../logger');
const configParser = require('../../config');
const repositoryWorker = require('../repository');
const versions = require('./versions');
+const configValidation = require('../../config/validation');
module.exports = {
start,
@@ -11,6 +12,10 @@ module.exports = {
async function start() {
try {
const config = await configParser.parseConfigs(process.env, process.argv);
+ const configErrors = configValidation.validateConfig(config);
+ if (configErrors.length) {
+ logger.error({ configErrors }, 'Found config errors');
+ }
config.logger = logger;
config.versions = versions.detectVersions(config);
// Iterate through repositories sequentially
diff --git a/lib/workers/package-file/index.js b/lib/workers/package-file/index.js
index 80a30cdc08..267d488a70 100644
--- a/lib/workers/package-file/index.js
+++ b/lib/workers/package-file/index.js
@@ -1,6 +1,7 @@
const path = require('path');
const configParser = require('../../config');
const depTypeWorker = require('../dep-type');
+const configValidation = require('../../config/validation');
let logger = require('../../logger');
@@ -11,6 +12,7 @@ module.exports = {
async function renovatePackageFile(packageFileConfig) {
let config = Object.assign({}, packageFileConfig);
+ let upgrades = [];
logger = config.logger;
logger.info(`Processing package file`);
// If onboarding, use the package.json in onboarding branch unless a custom base branch was defined
@@ -33,6 +35,23 @@ async function renovatePackageFile(packageFileConfig) {
{ config: packageContent.renovate },
'package.json>renovate config'
);
+ const errors = configValidation.validateConfig(packageContent.renovate);
+ if (errors.length) {
+ logger.warn(
+ { errors },
+ 'Found package.json>renovate configuration errors'
+ );
+ /* TODO #556
+ errors.forEach(error => {
+ upgrades.push(
+ Object.assign({}, error, {
+ depName: `${config.packageFile}(renovate)`,
+ type: 'error',
+ })
+ );
+ });
+ */
+ }
// package.json>renovate config takes precedence over existing config
config = configParser.mergeChildConfig(config, packageContent.renovate);
} else {
@@ -41,14 +60,13 @@ async function renovatePackageFile(packageFileConfig) {
// Now check if config is disabled
if (config.enabled === false) {
logger.info('packageFile is disabled');
- return [];
+ return upgrades;
}
const depTypeConfigs = config.depTypes.map(depType =>
module.exports.getDepTypeConfig(config, depType)
);
logger.trace({ config: depTypeConfigs }, `depTypeConfigs`);
- let upgrades = [];
for (const depTypeConfig of depTypeConfigs) {
upgrades = upgrades.concat(
await depTypeWorker.renovateDepType(packageContent, depTypeConfig)
diff --git a/lib/workers/repository/apis.js b/lib/workers/repository/apis.js
index 581705d778..97afea74b5 100644
--- a/lib/workers/repository/apis.js
+++ b/lib/workers/repository/apis.js
@@ -1,6 +1,7 @@
const ini = require('ini');
const jsonValidator = require('json-dup-key-validator');
const configParser = require('../../config');
+const configValidation = require('../../config/validation');
// API
const githubApi = require('../../api/github');
const gitlabApi = require('../../api/gitlab');
@@ -119,6 +120,16 @@ async function mergeRenovateJson(config, branchName) {
}
renovateJson = JSON.parse(renovateJsonContent);
config.logger.debug({ config: renovateJson }, 'renovate.json config');
+ const renovateJsonErrors = configValidation.validateConfig(renovateJson);
+ if (renovateJsonErrors.length) {
+ config.logger.warn({ renovateJsonErrors }, 'Found renovate.json errors');
+ /* TODO #556
+ renovateJsonErrors.forEach(error => {
+ config.errors.push(
+ Object.assign({}, error, { depName: 'renovate.json' })
+ );
+ }); */
+ }
returnConfig = configParser.mergeChildConfig(returnConfig, renovateJson);
returnConfig.renovateJsonPresent = true;
} catch (err) {
diff --git a/test/config/__snapshots__/validation.spec.js.snap b/test/config/__snapshots__/validation.spec.js.snap
new file mode 100644
index 0000000000..e893793b75
--- /dev/null
+++ b/test/config/__snapshots__/validation.spec.js.snap
@@ -0,0 +1,39 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`config/validation validateConfig(config) errors for all types 1`] = `
+Array [
+ Object {
+ "depName": "Configuration Error",
+ "message": "Configuration option \`enabled\` should be boolean",
+ },
+ Object {
+ "depName": "Configuration Error",
+ "message": "Configuration option \`schedule\` should be a list (Array)",
+ },
+ Object {
+ "depName": "Configuration Error",
+ "message": "Configuration option \`semanticPrefix\` should be a string",
+ },
+ Object {
+ "depName": "Configuration Error",
+ "message": "Configuration option \`githubAppId\` should be an integer",
+ },
+ Object {
+ "depName": "Configuration Error",
+ "message": "Configuration option \`lockFileMaintenance\` should be a json object",
+ },
+]
+`;
+
+exports[`config/validation validateConfig(config) returns nested errors 1`] = `
+Array [
+ Object {
+ "depName": "Configuration Error",
+ "message": "Invalid configuration option: \`foo\`",
+ },
+ Object {
+ "depName": "Configuration Error",
+ "message": "Invalid configuration option: \`bar\`",
+ },
+]
+`;
diff --git a/test/config/validation.spec.js b/test/config/validation.spec.js
new file mode 100644
index 0000000000..03b9854ed7
--- /dev/null
+++ b/test/config/validation.spec.js
@@ -0,0 +1,30 @@
+const configValidation = require('../../lib/config/validation.js');
+
+describe('config/validation', () => {
+ describe('validateConfig(config)', () => {
+ it('returns nested errors', () => {
+ const config = {
+ foo: 1,
+ prBody: 'some-body',
+ lockFileMaintenance: {
+ bar: 2,
+ },
+ };
+ const errors = configValidation.validateConfig(config);
+ expect(errors).toHaveLength(2);
+ expect(errors).toMatchSnapshot();
+ });
+ it('errors for all types', () => {
+ const config = {
+ enabled: 1,
+ schedule: 'after 5pm',
+ semanticPrefix: 7,
+ githubAppId: 'none',
+ lockFileMaintenance: false,
+ };
+ const errors = configValidation.validateConfig(config);
+ expect(errors).toHaveLength(5);
+ expect(errors).toMatchSnapshot();
+ });
+ });
+});
diff --git a/test/workers/global/index.spec.js b/test/workers/global/index.spec.js
index 96e753228b..551f8a41b9 100644
--- a/test/workers/global/index.spec.js
+++ b/test/workers/global/index.spec.js
@@ -11,6 +11,13 @@ describe('lib/workers/global', () => {
configParser.getRepositoryConfig = jest.fn();
repositoryWorker.renovateRepository = jest.fn();
});
+ it('handles config errors', async () => {
+ configParser.parseConfigs.mockReturnValueOnce({
+ repositories: [],
+ foo: 1,
+ });
+ await globalWorker.start();
+ });
it('handles zero repos', async () => {
configParser.parseConfigs.mockReturnValueOnce({
repositories: [],
@@ -19,7 +26,7 @@ describe('lib/workers/global', () => {
});
it('processes repositories', async () => {
configParser.parseConfigs.mockReturnValueOnce({
- foo: 1,
+ enabled: true,
repositories: ['a', 'b'],
});
await globalWorker.start();
diff --git a/test/workers/package-file/__snapshots__/index.spec.js.snap b/test/workers/package-file/__snapshots__/index.spec.js.snap
new file mode 100644
index 0000000000..c1631b6198
--- /dev/null
+++ b/test/workers/package-file/__snapshots__/index.spec.js.snap
@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`packageFileWorker renovatePackageFile(config) handles renovate config errors 1`] = `Array []`;
diff --git a/test/workers/package-file/index.spec.js b/test/workers/package-file/index.spec.js
index 3aa0e1761a..2141b83bf7 100644
--- a/test/workers/package-file/index.spec.js
+++ b/test/workers/package-file/index.spec.js
@@ -22,6 +22,12 @@ describe('packageFileWorker', () => {
logger,
});
});
+ it('handles renovate config errors', async () => {
+ config.enabled = false;
+ config.api.getFileJson.mockReturnValueOnce({ renovate: { foo: 1 } });
+ const res = await packageFileWorker.renovatePackageFile(config);
+ expect(res).toMatchSnapshot();
+ });
it('handles null', async () => {
const allUpgrades = await packageFileWorker.renovatePackageFile(config);
expect(allUpgrades).toHaveLength(1);
diff --git a/test/workers/repository/__snapshots__/apis.spec.js.snap b/test/workers/repository/__snapshots__/apis.spec.js.snap
index 37bbfa14db..c9858fc3fe 100644
--- a/test/workers/repository/__snapshots__/apis.spec.js.snap
+++ b/test/workers/repository/__snapshots__/apis.spec.js.snap
@@ -52,7 +52,7 @@ exports[`workers/repository/apis mergeRenovateJson(config) returns error in conf
Array [
Object {
"depName": "renovate.json",
- "message": "Syntax error: expecting String near { foo: 1 }",
+ "message": "Syntax error: expecting String near { enabled:",
},
]
`;
@@ -61,7 +61,9 @@ exports[`workers/repository/apis mergeRenovateJson(config) returns error plus ex
Array [
Object {
"depName": "renovate.json",
- "message": "Syntax error: duplicated keys \\"foo\\" near \\"foo\\": 2 }",
+ "message": "Syntax error: duplicated keys \\"enabled\\" near \\": false }",
},
]
`;
+
+exports[`workers/repository/apis mergeRenovateJson(config) returns error plus extended config if unknown keys 1`] = `Array []`;
diff --git a/test/workers/repository/apis.spec.js b/test/workers/repository/apis.spec.js
index 932562b4d7..4ece3f4b6d 100644
--- a/test/workers/repository/apis.spec.js
+++ b/test/workers/repository/apis.spec.js
@@ -129,29 +129,41 @@ describe('workers/repository/apis', () => {
expect(await apis.mergeRenovateJson(config)).toEqual(config);
});
it('returns extended config if renovate.json found', async () => {
- config.api.getFileContent.mockReturnValueOnce('{ "foo": 1 }');
+ config.api.getFileContent.mockReturnValueOnce('{ "enabled": true }');
const returnConfig = await apis.mergeRenovateJson(config);
- expect(returnConfig.foo).toBe(1);
+ expect(returnConfig.enabled).toBe(true);
expect(returnConfig.renovateJsonPresent).toBe(true);
expect(returnConfig.errors).toHaveLength(0);
});
+ it('returns error plus extended config if unknown keys', async () => {
+ config.api.getFileContent.mockReturnValueOnce(
+ '{ "enabled": true, "foo": false }'
+ );
+ const returnConfig = await apis.mergeRenovateJson(config);
+ expect(returnConfig.enabled).toBe(true);
+ expect(returnConfig.renovateJsonPresent).toBe(true);
+ expect(returnConfig.errors).toHaveLength(0); // TODO: Update to 1 later
+ expect(returnConfig.errors).toMatchSnapshot();
+ });
it('returns error plus extended config if duplicate keys', async () => {
- config.api.getFileContent.mockReturnValueOnce('{ "foo": 1, "foo": 2 }');
+ config.api.getFileContent.mockReturnValueOnce(
+ '{ "enabled": true, "enabled": false }'
+ );
const returnConfig = await apis.mergeRenovateJson(config);
- expect(returnConfig.foo).toBe(2);
+ expect(returnConfig.enabled).toBe(false);
expect(returnConfig.renovateJsonPresent).toBe(true);
expect(returnConfig.errors).toHaveLength(1);
expect(returnConfig.errors).toMatchSnapshot();
});
it('returns error in config if renovate.json cannot be parsed', async () => {
- config.api.getFileContent.mockReturnValueOnce('{ foo: 1 }');
+ config.api.getFileContent.mockReturnValueOnce('{ enabled: true }');
const returnConfig = await apis.mergeRenovateJson(config);
- expect(returnConfig.foo).toBeUndefined();
+ expect(returnConfig.enabled).toBeUndefined();
expect(returnConfig.renovateJsonPresent).toBeUndefined();
expect(returnConfig.errors).toMatchSnapshot();
});
it('returns error in JSON.parse', async () => {
- config.api.getFileContent.mockReturnValueOnce('{ foo: 1 }');
+ config.api.getFileContent.mockReturnValueOnce('{ enabled: true }');
jsonValidator.validate = jest.fn();
jsonValidator.validate.mockReturnValueOnce(false);
jsonValidator.validate.mockReturnValueOnce(false);
--
GitLab