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

@@ -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;
+}

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

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)

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) {

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\`",
+  },
+]
+`;

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();
+    });
+  });
+});

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();

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 []`;

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);

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 []`;

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 duplicate keys', async () => {
-      config.api.getFileContent.mockReturnValueOnce('{ "foo": 1, "foo": 2 }');
+    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.foo).toBe(2);
+      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(
+        '{ "enabled": true, "enabled": false }'
+      );
+      const returnConfig = await apis.mergeRenovateJson(config);
+      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);