Merge pull request #4419 from HyperBrain/fix-function-configuration-update

Do not update function configuration parts that contain references.

lib/plugins/aws/deployFunction/index.js

@@ -122,29 +122,29 @@ class AwsDeployFunction {
       FunctionName: functionObj.name,
     };
 
-    if ('awsKmsKeyArn' in functionObj) {
+    if ('awsKmsKeyArn' in functionObj && !_.isObject(functionObj.awsKmsKeyArn)) {
       params.KMSKeyArn = functionObj.awsKmsKeyArn;
-    } else if (serviceObj && 'awsKmsKeyArn' in serviceObj) {
+    } else if (serviceObj && 'awsKmsKeyArn' in serviceObj && !_.isObject(serviceObj.awsKmsKeyArn)) {
       params.KMSKeyArn = serviceObj.awsKmsKeyArn;
     }
 
-    if ('description' in functionObj) {
+    if ('description' in functionObj && !_.isObject(functionObj.description)) {
       params.Description = functionObj.description;
     }
 
-    if ('memorySize' in functionObj) {
+    if ('memorySize' in functionObj && !_.isObject(functionObj.memorySize)) {
       params.MemorySize = functionObj.memorySize;
-    } else if ('memorySize' in providerObj) {
+    } else if ('memorySize' in providerObj && !_.isObject(providerObj.memorySize)) {
       params.MemorySize = providerObj.memorySize;
     }
 
-    if ('timeout' in functionObj) {
+    if ('timeout' in functionObj && !_.isObject(functionObj.timeout)) {
       params.Timeout = functionObj.timeout;
-    } else if ('timeout' in providerObj) {
+    } else if ('timeout' in providerObj && !_.isObject(providerObj.timeout)) {
       params.Timeout = providerObj.timeout;
     }
 
-    if (functionObj.onError) {
+    if (functionObj.onError && !_.isObject(functionObj.onError)) {
       params.DeadLetterConfig = {
         TargetArn: functionObj.onError,
       };
@@ -158,38 +158,43 @@ class AwsDeployFunction {
         functionObj.environment
       );
 
-      Object.keys(params.Environment.Variables).forEach((key) => {
-        // taken from the bash man pages
-        if (!key.match(/^[A-Za-z_][a-zA-Z0-9_]*$/)) {
-          const errorMessage = 'Invalid characters in environment variable';
-          throw new this.serverless.classes.Error(errorMessage);
-        }
-      });
+      if (_.some(params.Environment.Variables, value => _.isObject(value))) {
+        delete params.Environment;
+      } else {
+        Object.keys(params.Environment.Variables).forEach((key) => {
+          // taken from the bash man pages
+          if (!key.match(/^[A-Za-z_][a-zA-Z0-9_]*$/)) {
+            const errorMessage = 'Invalid characters in environment variable';
+            throw new this.serverless.classes.Error(errorMessage);
+          }
+        });
+      }
     }
 
     if (functionObj.vpc || providerObj.vpc) {
+      const vpc = functionObj.vpc || providerObj.vpc;
       params.VpcConfig = {};
+
+      if (_.isArray(vpc.securityGroupIds) && !_.some(vpc.securityGroupIds, _.isObject)) {
+        params.VpcConfig.SecurityGroupIds = vpc.securityGroupIds;
+      }
+
+      if (_.isArray(vpc.subnetIds) && !_.some(vpc.subnetIds, _.isObject)) {
+        params.VpcConfig.SubnetIds = vpc.subnetIds;
+      }
+
+      if (_.isEmpty(params.VpcConfig)) {
+        delete params.VpcConfig;
+      }
     }
 
-    if (functionObj.vpc && functionObj.vpc.securityGroupIds) {
-      params.VpcConfig.SecurityGroupIds = functionObj.vpc.securityGroupIds;
-    } else if (providerObj.vpc && providerObj.vpc.securityGroupIds) {
-      params.VpcConfig.SecurityGroupIds = providerObj.vpc.securityGroupIds;
-    }
-
-    if (functionObj.vpc && functionObj.vpc.subnetIds) {
-      params.VpcConfig.SubnetIds = functionObj.vpc.subnetIds;
-    } else if (providerObj.vpc && providerObj.vpc.subnetIds) {
-      params.VpcConfig.SubnetIds = providerObj.vpc.subnetIds;
-    }
-
-    if ('role' in functionObj) {
+    if ('role' in functionObj && !_.isObject(functionObj.role)) {
       return this.normalizeArnRole(functionObj.role).then(roleArn => {
         params.Role = roleArn;
 
         return this.callUpdateFunctionConfiguration(params);
       });
-    } else if ('role' in providerObj) {
+    } else if ('role' in providerObj && !_.isObject(providerObj.role)) {
       return this.normalizeArnRole(providerObj.role).then(roleArn => {
         params.Role = roleArn;
 
@@ -197,6 +202,10 @@ class AwsDeployFunction {
       });
     }
 
+    if (_.isEmpty(_.omit(params, 'FunctionName'))) {
+      return BbPromise.resolve();
+    }
+
     return this.callUpdateFunctionConfiguration(params);
   }
 

lib/plugins/aws/deployFunction/index.test.js

@@ -1,6 +1,6 @@
 'use strict';
 
-const expect = require('chai').expect;
+const chai = require('chai');
 const sinon = require('sinon');
 const path = require('path');
 const fs = require('fs');
@@ -9,6 +9,11 @@ const AwsProvider = require('../provider/awsProvider');
 const Serverless = require('../../../Serverless');
 const testUtils = require('../../../../tests/utils');
 
+chai.use(require('chai-as-promised'));
+chai.use(require('sinon-chai'));
+
+const expect = chai.expect;
+
 describe('AwsDeployFunction', () => {
   let AwsDeployFunction;
   let serverless;
@@ -288,6 +293,60 @@ describe('AwsDeployFunction', () => {
       });
     });
 
+    it('should skip elements that contain references', () => {
+      options.functionObj = {
+        name: 'first',
+        description: 'change',
+        vpc: {
+          securityGroupIds: ['xxxxx', {
+            ref: 'myVPCRef',
+          }],
+          subnetIds: ['xxxxx', {
+            ref: 'myVPCRef',
+          }],
+        },
+        environment: {
+          myvar: 'this is my var',
+          myref: {
+            ref: 'aCFReference',
+          },
+        },
+      };
+
+      awsDeployFunction.options = options;
+
+      return awsDeployFunction.updateFunctionConfiguration().then(() => {
+        expect(updateFunctionConfigurationStub.calledOnce).to.be.equal(true);
+        expect(updateFunctionConfigurationStub.calledWithExactly(
+          'Lambda',
+          'updateFunctionConfiguration',
+          {
+            FunctionName: 'first',
+            Description: 'change',
+          },
+          awsDeployFunction.options.stage,
+          awsDeployFunction.options.region
+        )).to.be.equal(true);
+      });
+    });
+
+    it('should do nothing if only references are in', () => {
+      options.functionObj = {
+        name: 'first',
+        environment: {
+          myvar: 'this is my var',
+          myref: {
+            ref: 'aCFReference',
+          },
+        },
+      };
+
+      awsDeployFunction.options = options;
+
+      return expect(awsDeployFunction.updateFunctionConfiguration()).to.be.fulfilled
+      .then(() => expect(updateFunctionConfigurationStub).to.not.be.called);
+    });
+
     it('should fail when using invalid characters in environment variable', () => {
       options.functionObj = {
         name: 'first',