From 97896eb34fa3282f42c0cc72d2946c94e284731c Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Thu, 26 Oct 2017 11:40:17 +0200 Subject: [PATCH 1/2] Do not update configuration parts that contain references. --- lib/plugins/aws/deployFunction/index.js | 67 +++++++++++--------- lib/plugins/aws/deployFunction/index.test.js | 30 +++++++++ 2 files changed, 68 insertions(+), 29 deletions(-) diff --git a/lib/plugins/aws/deployFunction/index.js b/lib/plugins/aws/deployFunction/index.js index 4d347b8d9..61db150e3 100644 --- a/lib/plugins/aws/deployFunction/index.js +++ b/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); } diff --git a/lib/plugins/aws/deployFunction/index.test.js b/lib/plugins/aws/deployFunction/index.test.js index 5e6def27f..7a1ada4a9 100644 --- a/lib/plugins/aws/deployFunction/index.test.js +++ b/lib/plugins/aws/deployFunction/index.test.js @@ -288,6 +288,36 @@ describe('AwsDeployFunction', () => { }); }); + it('should skip elements that contain references', () => { + options.functionObj = { + name: 'first', + description: 'change', + vpc: undefined, + 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 fail when using invalid characters in environment variable', () => { options.functionObj = { name: 'first', From 06cf6df2e6c8cd330b480cfa934082f7d3eb946f Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Thu, 26 Oct 2017 12:14:10 +0200 Subject: [PATCH 2/2] More tests to increase coverage --- lib/plugins/aws/deployFunction/index.test.js | 33 ++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/plugins/aws/deployFunction/index.test.js b/lib/plugins/aws/deployFunction/index.test.js index 7a1ada4a9..886d61fdd 100644 --- a/lib/plugins/aws/deployFunction/index.test.js +++ b/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; @@ -292,7 +297,14 @@ describe('AwsDeployFunction', () => { options.functionObj = { name: 'first', description: 'change', - vpc: undefined, + vpc: { + securityGroupIds: ['xxxxx', { + ref: 'myVPCRef', + }], + subnetIds: ['xxxxx', { + ref: 'myVPCRef', + }], + }, environment: { myvar: 'this is my var', myref: { @@ -318,6 +330,23 @@ describe('AwsDeployFunction', () => { }); }); + 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',