feat: validate schedule (#674)

Also enables config errors to be shown in Configure Renovate PR.

Closes #550

lib/config/validation.js

 const options = require('./definitions').getOptions();
+const { hasValidSchedule } = require('../workers/branch/schedule');
 
 const optionTypes = {};
 options.forEach(option => {
@@ -42,6 +43,14 @@ function validateConfig(config) {
           depName: 'Configuration Error',
           message: `Invalid configuration option: \`${key}\``,
         });
+      } else if (key === 'schedule') {
+        const [validSchedule, errorMessage] = hasValidSchedule(val);
+        if (!validSchedule) {
+          errors.push({
+            depName: 'Configuration Error',
+            message: `Invalid schedule: \`${errorMessage}\``,
+          });
+        }
       } else if (val != null) {
         const type = optionTypes[key];
         if (type === 'boolean') {

lib/workers/branch/schedule.js

@@ -10,29 +10,25 @@ function fixShortHours(input) {
   return input.replace(/( \d?\d)((a|p)m)/g, '$1:00$2');
 }
 
-function hasValidSchedule(schedule, logger) {
+function hasValidSchedule(schedule) {
   if (!Array.isArray(schedule)) {
-    logger.debug({ schedule }, `Invalid schedule`);
-    return false;
-  }
-  if (schedule.length === 0) {
-    logger.debug('No schedule defined');
-    return false;
-  }
-  if (schedule.length === 1 && schedule[0] === '') {
-    logger.debug('Empty schedule');
-    return false;
+    return [false, 'Invalid schedule'];
   }
+  let message;
   // check if any of the schedules fail to parse
   const hasFailedSchedules = schedule.some(scheduleText => {
     const parsedSchedule = later.parse.text(fixShortHours(scheduleText));
     if (parsedSchedule.error !== -1) {
-      logger.debug(`Failed to parse schedule ${scheduleText}`);
+      message = `Failed to parse schedule ${scheduleText}`;
       // It failed to parse
       return true;
     }
+    if (parsedSchedule.schedules.some(s => s.m)) {
+      message = 'Schedules should not specify minutes';
+      return true;
+    }
     if (!parsedSchedule.schedules.some(s => s.d || s.t_a || s.t_b)) {
-      logger.debug('Schedule has no days of week or time of day');
+      message = 'Schedule has no days of week or time of day';
       return true;
     }
     // It must be OK
@@ -40,15 +36,24 @@ function hasValidSchedule(schedule, logger) {
   });
   if (hasFailedSchedules) {
     // If any fail then we invalidate the whole thing
-    return false;
+    return [false, message];
   }
-  return true;
+  return [true];
 }
 
 function isScheduledNow(config) {
   config.logger.debug({ schedule: config.schedule }, `Checking schedule`);
-  if (!module.exports.hasValidSchedule(config.schedule, config.logger)) {
-    // Return true if the schedule is invalid
+  if (
+    !config.schedule ||
+    config.schedule.length === 0 ||
+    config.schedule[0] === ''
+  ) {
+    config.logger.debug('No schedule defined');
+    return true;
+  }
+  const [validSchedule, errorMessage] = hasValidSchedule(config.schedule);
+  if (!validSchedule) {
+    config.logger.error(errorMessage);
     return true;
   }
   let now = moment();

lib/workers/repository/apis.js

@@ -124,6 +124,10 @@ function migrateAndValidate(config, input) {
       );
     }); */
   }
+  if (!config.repoIsOnboarded) {
+    migratedConfig.warnings = (migratedConfig.warnings || []).concat(warnings);
+    migratedConfig.errors = (migratedConfig.errors || []).concat(errors);
+  }
   return migratedConfig;
 }
 

test/config/__snapshots__/validation.spec.js.snap

@@ -8,7 +8,11 @@ Array [
   },
   Object {
     "depName": "Configuration Error",
-    "message": "Configuration option \`schedule\` should be a list (Array)",
+    "message": "Invalid schedule: \`Schedules should not specify minutes\`",
+  },
+  Object {
+    "depName": "Configuration Error",
+    "message": "Configuration option \`labels\` should be a list (Array)",
   },
   Object {
     "depName": "Configuration Error",

test/config/validation.spec.js

 const configValidation = require('../../lib/config/validation.js');
+const logger = require('../_fixtures/logger');
 
 describe('config/validation', () => {
   describe('validateConfig(config)', () => {
@@ -19,7 +20,8 @@ describe('config/validation', () => {
     it('errors for all types', () => {
       const config = {
         enabled: 1,
-        schedule: 5,
+        schedule: ['every 15 mins every weekday'],
+        labels: 5,
         semanticPrefix: 7,
         githubAppId: 'none',
         lockFileMaintenance: false,
@@ -29,9 +31,12 @@ describe('config/validation', () => {
           },
         ],
       };
-      const { warnings, errors } = configValidation.validateConfig(config);
+      const { warnings, errors } = configValidation.validateConfig(
+        config,
+        logger
+      );
       expect(warnings).toHaveLength(0);
-      expect(errors).toHaveLength(6);
+      expect(errors).toHaveLength(7);
       expect(errors).toMatchSnapshot();
     });
   });

test/workers/branch/schedule.spec.js

@@ -8,44 +8,48 @@ describe('workers/branch/schedule', () => {
       jest.resetAllMocks();
     });
     it('returns false if schedule is not an array', () => {
-      expect(schedule.hasValidSchedule({ a: 1 }, logger)).toBe(false);
-    });
-    it('returns false if schedule array is empty', () => {
-      expect(schedule.hasValidSchedule([], logger)).toBe(false);
-    });
-    it('returns false if only schedule is empty', () => {
-      expect(schedule.hasValidSchedule([''], logger)).toBe(false);
+      expect(schedule.hasValidSchedule({ a: 1 }, logger)[0]).toBe(false);
     });
     it('returns false for invalid schedule', () => {
-      expect(schedule.hasValidSchedule(['foo'], logger)).toBe(false);
+      expect(schedule.hasValidSchedule(['foo'], logger)[0]).toBe(false);
     });
     it('returns false if any schedule fails to parse', () => {
-      expect(schedule.hasValidSchedule(['after 5:00pm', 'foo'], logger)).toBe(
-        false
-      );
+      expect(
+        schedule.hasValidSchedule(['after 5:00pm', 'foo'], logger)[0]
+      ).toBe(false);
+    });
+    it('returns false if using minutes', () => {
+      expect(
+        schedule.hasValidSchedule(['every 15 mins every weekday'], logger)[0]
+      ).toBe(false);
     });
     it('returns false if schedules have no days or time range', () => {
-      expect(schedule.hasValidSchedule(['at 5:00pm'], logger)).toBe(false);
+      expect(schedule.hasValidSchedule(['at 5:00pm'], logger)[0]).toBe(false);
     });
     it('returns false if any schedule has no days or time range', () => {
       expect(
-        schedule.hasValidSchedule(['at 5:00pm', 'on saturday'], logger)
+        schedule.hasValidSchedule(['at 5:00pm', 'on saturday'], logger)[0]
       ).toBe(false);
     });
     it('returns true if schedule has days of week', () => {
       expect(
-        schedule.hasValidSchedule(['on friday and saturday'], logger)
+        schedule.hasValidSchedule(['on friday and saturday'], logger)[0]
       ).toBe(true);
     });
     it('returns true if schedule has a start time', () => {
-      expect(schedule.hasValidSchedule(['after 8:00pm'], logger)).toBe(true);
+      expect(schedule.hasValidSchedule(['after 8:00pm'], logger)[0]).toBe(true);
     });
     it('returns true if schedule has an end time', () => {
-      expect(schedule.hasValidSchedule(['before 6:00am'], logger)).toBe(true);
+      expect(schedule.hasValidSchedule(['before 6:00am'], logger)[0]).toBe(
+        true
+      );
     });
     it('returns true if schedule has a start and end time', () => {
       expect(
-        schedule.hasValidSchedule(['after 11:00pm and before 6:00am'], logger)
+        schedule.hasValidSchedule(
+          ['after 11:00pm and before 6:00am'],
+          logger
+        )[0]
       ).toBe(true);
     });
     it('returns true if schedule has days and a start and end time', () => {
@@ -53,11 +57,11 @@ describe('workers/branch/schedule', () => {
         schedule.hasValidSchedule(
           ['after 11:00pm and before 6:00am every weekday'],
           logger
-        )
+        )[0]
       ).toBe(true);
     });
     it('supports hours shorthand', () => {
-      const res = schedule.hasValidSchedule(
+      const [res] = schedule.hasValidSchedule(
         [
           'after 11pm and before 6am every weekend',
           'after 11pm',
@@ -86,6 +90,11 @@ describe('workers/branch/schedule', () => {
       const res = schedule.isScheduledNow(config);
       expect(res).toBe(true);
     });
+    it('returns true if invalid schedule', () => {
+      config.schedule = ['every 15 minutes'];
+      const res = schedule.isScheduledNow(config);
+      expect(res).toBe(true);
+    });
     it('supports before hours true', () => {
       config.schedule = ['before 4:00pm'];
       const res = schedule.isScheduledNow(config);

test/workers/repository/__snapshots__/apis.spec.js.snap

@@ -78,9 +78,11 @@ exports[`workers/repository/apis resolvePackageFiles includes files with content
 Array [
   Object {
     "content": Object {},
+    "errors": Array [],
     "hasPackageLock": true,
     "hasYarnLock": true,
     "packageFile": "package.json",
+    "warnings": Array [],
   },
   Object {
     "content": Object {},

test/workers/repository/apis.spec.js

@@ -159,6 +159,7 @@ describe('workers/repository/apis', () => {
       expect(returnConfig.errors).toHaveLength(0);
     });
     it('returns warning + error plus extended config if unknown keys', async () => {
+      config.repoIsOnboarded = true;
       config.api.getFileContent.mockReturnValueOnce(
         '{ "enabled": true, "foo": false, "maintainYarnLock": true, "schedule": "before 5am", "minor": {} }'
       );
@@ -169,6 +170,7 @@ describe('workers/repository/apis', () => {
       expect(returnConfig.errors).toMatchSnapshot();
     });
     it('returns error plus extended config if duplicate keys', async () => {
+      config.repoIsOnboarded = true;
       config.api.getFileContent.mockReturnValueOnce(
         '{ "enabled": true, "enabled": false }'
       );