Fix error when trying to override concurrency option via CLI option

lib/Configuration.js

@@ -15,7 +15,7 @@ const DefaultConfig = {
   packagerOptions: {},
   keepOutputDirectory: false,
   config: null,
-  concurrency: undefined
+  concurrency: Infinity
 };
 
 class Configuration {
@@ -38,6 +38,21 @@ class Configuration {
       }
     }
 
+    // Concurrency may be passed via CLI, e.g.
+    // custom:
+    //   webpack:
+    //     concurrency: ${opt:compile-concurrency, 7}
+    // In this case it is typed as a string and we have to validate it
+    if (this._config.concurrency !== undefined) {
+      this._config.concurrency = Number(this._config.concurrency);
+      if (isNaN(this._config.concurrency) || this._config.concurrency < 1) {
+        throw new Error('concurrency option must be a positive number');
+      }
+    } else if (this._config.serializedCompile === true) {
+      // Backwards compatibility with serializedCompile setting
+      this._config.concurrency = 1;
+    }
+
     // Set defaults for all missing properties
     _.defaults(this._config, DefaultConfig);
   }
@@ -79,11 +94,7 @@ class Configuration {
   }
 
   get concurrency() {
-    if (this._config.concurrency !== undefined) {
-      return this._config.concurrency;
-    } else if (this._config.serializedCompile === true) {
-      return 1;
-    }
+    return this._config.concurrency;
   }
 
   toJSON() {

lib/Configuration.test.js

@@ -20,7 +20,7 @@ describe('Configuration', () => {
         packagerOptions: {},
         keepOutputDirectory: false,
         config: null,
-        concurrency: undefined
+        concurrency: Infinity
       };
     });
 
@@ -80,7 +80,7 @@ describe('Configuration', () => {
         packagerOptions: {},
         keepOutputDirectory: false,
         config: null,
-        concurrency: undefined
+        concurrency: Infinity
       });
     });
   });
@@ -101,7 +101,7 @@ describe('Configuration', () => {
         packagerOptions: {},
         keepOutputDirectory: false,
         config: null,
-        concurrency: undefined
+        concurrency: Infinity
       });
     });
 
@@ -121,8 +121,42 @@ describe('Configuration', () => {
         packagerOptions: {},
         keepOutputDirectory: false,
         config: null,
-        concurrency: undefined
+        concurrency: Infinity
       });
     });
+
+    it('should accept a numeric string as concurrency value', () => {
+      const testCustom = {
+        webpack: {
+          includeModules: { forceInclude: ['mod1'] },
+          webpackConfig: 'myWebpackFile.js',
+          concurrency: '3'
+        }
+      };
+      const config = new Configuration(testCustom);
+      expect(config._config.concurrency).to.equal(3);
+    });
+
+    it('should not accept an invalid string as concurrency value', () => {
+      const testCustom = {
+        webpack: {
+          includeModules: { forceInclude: ['mod1'] },
+          webpackConfig: 'myWebpackFile.js',
+          concurrency: '3abc'
+        }
+      };
+      expect(() => new Configuration(testCustom)).throws();
+    });
+
+    it('should not accept a non-positive number as concurrency value', () => {
+      const testCustom = {
+        webpack: {
+          includeModules: { forceInclude: ['mod1'] },
+          webpackConfig: 'myWebpackFile.js',
+          concurrency: 0
+        }
+      };
+      expect(() => new Configuration(testCustom)).throws();
+    });
   });
 });

lib/compile.js

@@ -51,7 +51,11 @@ module.exports = {
 
     const configs = ensureArray(this.webpackConfig);
     const logStats = getStatsLogger(configs[0].stats, this.serverless.cli.consoleLog);
-    const concurrency = this.concurrency === undefined ? Infinity : this.concurrency;
+
+    if (!this.configuration) {
+      return BbPromise.reject('Missing plugin configuration');
+    }
+    const concurrency = this.configuration.concurrency;
 
     return webpackConcurrentCompile(configs, logStats, concurrency)
       .then(stats => {

lib/validate.js

@@ -179,8 +179,9 @@ module.exports = {
       // In case of individual packaging we have to create a separate config for each function
       if (_.has(this.serverless, 'service.package') && this.serverless.service.package.individually) {
         this.multiCompile = true;
-        this.concurrency = this.configuration.concurrency;
-        this.options.verbose && this.serverless.cli.log(`Using ${this.concurrency ? 'concurrent' : 'multi'}-compile (individual packaging)`);
+        this.options.verbose && this.serverless.cli.log(
+          `Using ${this.configuration.concurrency !== Infinity ? 'concurrent' : 'multi'}-compile (individual packaging)`
+        );
 
         if (this.webpackConfig.entry && !_.isEqual(this.webpackConfig.entry, entries)) {
           return BbPromise.reject(

tests/compile.test.js

@@ -65,15 +65,24 @@ describe('compile', () => {
   it('should compile with webpack from a context configuration', () => {
     const testWebpackConfig = 'testconfig';
     module.webpackConfig = testWebpackConfig;
+    module.configuration = { concurrency: Infinity };
     return expect(module.compile()).to.be.fulfilled.then(() => {
       expect(webpackMock).to.have.been.calledWith(testWebpackConfig);
       expect(webpackMock.compilerMock.run).to.have.been.calledOnce;
       return null;
     });
   });
 
+  it('should fail if configuration is missing', () => {
+    const testWebpackConfig = 'testconfig';
+    module.webpackConfig = testWebpackConfig;
+    module.configuration = undefined;
+    return expect(module.compile()).to.be.rejectedWith('Missing plugin configuration');
+  });
+
   it('should fail if there are compilation errors', () => {
     module.webpackConfig = 'testconfig';
+    module.configuration = { concurrency: Infinity };
     // We stub errors here. It will be reset again in afterEach()
     sandbox.stub(webpackMock.statsMock.compilation, 'errors').value(['error']);
     return expect(module.compile()).to.be.rejectedWith(/compilation error/);
@@ -97,6 +106,7 @@ describe('compile', () => {
     };
     module.webpackConfig = testWebpackConfig;
     module.multiCompile = true;
+    module.configuration = { concurrency: Infinity };
     webpackMock.compilerMock.run.reset();
     webpackMock.compilerMock.run.yields(null, multiStats);
     return expect(module.compile()).to.be.fulfilled.then(() => {
@@ -124,7 +134,7 @@ describe('compile', () => {
     };
     module.webpackConfig = testWebpackConfig;
     module.multiCompile = true;
-    module.concurrency = 1;
+    module.configuration = { concurrency: 1 };
     webpackMock.compilerMock.run.reset();
     webpackMock.compilerMock.run.yields(null, multiStats);
     return expect(module.compile()).to.be.fulfilled.then(() => {
@@ -150,6 +160,7 @@ describe('compile', () => {
     };
 
     module.webpackConfig = testWebpackConfig;
+    module.configuration = { concurrency: Infinity };
     webpackMock.compilerMock.run.reset();
     webpackMock.compilerMock.run.yields(null, mockStats);
     return expect(module.compile())