Skip to content

Commit

Permalink
chore(all): reduce cognitive complexity (#1009)
Browse files Browse the repository at this point in the history
* extract s3 and sqs prop checking

* removed alb bucket check from CheckS3Props

* Check VPC and S3 props
  • Loading branch information
biffgaut committed Sep 15, 2023
1 parent beda875 commit 73172bb
Show file tree
Hide file tree
Showing 91 changed files with 1,216 additions and 641 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export class AlbToFargate extends Construct {
defaults.CheckProps(props);
defaults.CheckAlbProps(props);
defaults.CheckFargateProps(props);
defaults.CheckVpcProps(props);

// Obtain VPC for construct (existing or created)
this.vpc = defaults.buildVpc(scope, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -463,3 +463,24 @@ test('Test HTTPS API with new vpc, load balancer, service and private API', () =
]
});
});

test('Confirm that CheckVpcProps is called', () => {
const stack = new cdk.Stack(undefined, undefined, {
env: { account: "123456789012", region: 'us-east-1' },
});

const props: AlbToFargateProps = {
ecrRepositoryArn: defaults.fakeEcrRepoArn,
listenerProps: {
certificates: [defaults.getFakeCertificate(stack, "fake-cert")]
},
publicApi: false,
vpcProps: {},
existingVpc: defaults.getTestVpc(stack),
};
const app = () => {
new AlbToFargate(stack, 'new-construct', props);
};
// Assertion
expect(app).toThrowError('Error - Either provide an existingVpc or some combination of deployVpc and vpcProps, but not both.\n');
});
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ export class AlbToLambda extends Construct {
super(scope, id);
defaults.CheckProps(props);
defaults.CheckAlbProps(props);
defaults.CheckVpcProps(props);

// Obtain VPC for construct (existing or created)
this.vpc = defaults.buildVpc(scope, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ test("Test existing load balancer and existing lambda function", () => {
existingLambdaObj: lambdaFunction,
existingLoadBalancerObj: existingAlb,
listenerProps: {
certificates: [ defaults.getFakeCertificate(stack, "fake-cert") ],
certificates: [defaults.getFakeCertificate(stack, "fake-cert")],
},
publicApi: true,
existingVpc: testExistingVpc,
Expand Down Expand Up @@ -289,7 +289,7 @@ test('Test new load balancer and new lambda function', () => {
});

const props: AlbToLambdaProps = {
lambdaFunctionProps: {
lambdaFunctionProps: {
code: lambda.Code.fromAsset(`${__dirname}/lambda`),
runtime: lambda.Runtime.NODEJS_16_X,
handler: 'index.handler',
Expand Down Expand Up @@ -952,3 +952,28 @@ test('Test existingLoadBalancerObj and no existingVpc is an error', () => {
expect(app).toThrowError(
/An existing ALB is already in a VPC, that VPC must be provided in props.existingVpc for the rest of the construct to use./);
});

test('Confirm that CheckVpcProps is called', () => {
const stack = new cdk.Stack(undefined, undefined, {
env: { account: "123456789012", region: 'us-east-1' },
});

const props: AlbToLambdaProps = {
lambdaFunctionProps: {
code: lambda.Code.fromAsset(`${__dirname}/lambda`),
runtime: lambda.Runtime.NODEJS_16_X,
handler: 'index.handler'
},
listenerProps: {
certificates: [defaults.getFakeCertificate(stack, "fake-cert")]
},
publicApi: false,
vpcProps: {},
existingVpc: defaults.getTestVpc(stack),
};
const app = () => {
new AlbToLambda(stack, 'new-construct', props);
};
// Assertion
expect(app).toThrowError('Error - Either provide an existingVpc or some combination of deployVpc and vpcProps, but not both.\n');
});
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ export class ApiGatewayToSqs extends Construct {
constructor(scope: Construct, id: string, props: ApiGatewayToSqsProps) {
super(scope, id);
defaults.CheckProps(props);
defaults.CheckSqsProps(props);

if (this.CheckCreateRequestProps(props)) {
throw new Error(`The 'allowCreateOperation' property must be set to true when setting any of the following: ` +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*/

// Imports
import { Stack } from "aws-cdk-lib";
import { RemovalPolicy, Stack } from "aws-cdk-lib";
import { ApiGatewayToSqs } from '../lib';
import * as api from "aws-cdk-lib/aws-apigateway";
import * as kms from 'aws-cdk-lib/aws-kms';
Expand Down Expand Up @@ -634,4 +634,19 @@ test('Construct throws error when deleteIntegrationResponses is set and allowDel
});

expect(app).toThrowError(/The 'allowDeleteOperation' property must be set to true when setting any of the following: 'deleteRequestTemplate', 'additionalDeleteRequestTemplates', 'deleteIntegrationResponses'/);
});
});

test('Confirm the CheckSqsProps is being called', () => {
const stack = new Stack();

const app = () => {
new ApiGatewayToSqs(stack, 'api-gateway-sqs', {
queueProps: {
removalPolicy: RemovalPolicy.DESTROY,
},
existingQueueObj: new sqs.Queue(stack, 'test', {})
});
};

expect(app).toThrowError(/Error - Either provide queueProps or existingQueueObj, but not both.\n/);
});
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ export class CloudFrontToS3 extends Construct {
this.node.setContext("@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy", true);

defaults.CheckProps(props);
defaults.CheckS3Props(props);

let bucket: s3.IBucket;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,7 @@ test('check properties', () => {
expect(construct.s3Bucket !== null);
});

// --------------------------------------------------------------
// Test bad call with existingBucket and bucketProps
// --------------------------------------------------------------
test("Test bad call with existingBucket and bucketProps", () => {
test("Confirm CheckS3Props is called", () => {
// Stack
const stack = new cdk.Stack();

Expand All @@ -166,7 +163,7 @@ test("Test bad call with existingBucket and bucketProps", () => {
});
};
// Assertion
expect(app).toThrowError();
expect(app).toThrowError('Error - Either provide bucketProps or existingBucketObj, but not both.\n');
});

test("Test existingBucketObj", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,22 @@ test('Test minimal deployment with an existing isolated VPC', () => {

template.resourceCountIs("AWS::EC2::VPC", 1);
expect(construct.vpc).toBeDefined();
});
});

test('Confirm CheckVpcProps is being called', () => {
const stack = new cdk.Stack();

const app = () => {
new DynamoDBStreamsToLambdaToElasticSearchAndKibana(stack, 'test-construct', {
lambdaFunctionProps: getDefaultTestLambdaProps(),
domainName: "test",
deployVpc: true,
vpcProps: {
vpcName: "existing-vpc-test"
},
existingVpc: defaults.getTestVpc(stack),
});
};

expect(app).toThrowError('Error - Either provide an existingVpc or some combination of deployVpc and vpcProps, but not both.\n');
});
Original file line number Diff line number Diff line change
Expand Up @@ -291,3 +291,23 @@ test('Supply an existing logging bucket', () => {
template.resourceCountIs("AWS::S3::Bucket", 2);

});

test('Confirm CheckS3Props is being called', () => {
// For L4 constructs, the call is within the internal L3 constructs
const stack = new cdk.Stack();

const props: EventbridgeToKinesisFirehoseToS3Props = {
eventRuleProps: {
eventPattern: {
source: ['solutionsconstructs']
}
},
bucketProps: {},
existingBucketObj: new s3.Bucket(stack, 'test-bucket', {}),
};

const app = () => {
new EventbridgeToKinesisFirehoseToS3(stack, 'test-eventbridge-firehose', props);
};
expect(app).toThrowError('Error - Either provide bucketProps or existingBucketObj, but not both.\n');
});
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export class EventbridgeToSns extends Construct {
constructor(scope: Construct, id: string, props: EventbridgeToSnsProps) {
super(scope, id);
defaults.CheckProps(props);
defaults.CheckSnsProps(props);

let enableEncryptionParam = props.enableEncryptionWithCustomerManagedKey;
if (props.enableEncryptionWithCustomerManagedKey === undefined ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -390,4 +390,24 @@ test('Properties correctly set when unencrypted existing topic is provided', ()

expect(testConstruct.snsTopic).toBeDefined();
expect(testConstruct.encryptionKey).not.toBeDefined();
});

test('Confirm CheckSnsProps is being called', () => {
const stack = new cdk.Stack();
const existingTopicObj = new sns.Topic(stack, 'Topic', {
topicName: 'existing-topic-name'
});

const props: EventbridgeToSnsProps = {
existingTopicObj,
topicProps: {},
eventRuleProps: {
schedule: events.Schedule.rate(cdk.Duration.minutes(5))
}
};
const app = () => {
new EventbridgeToSns(stack, 'test', props);
};

expect(app).toThrowError("Error - Either provide topicProps or existingTopicObj, but not both.\n");
});
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export class EventbridgeToSqs extends Construct {
constructor(scope: Construct, id: string, props: EventbridgeToSqsProps) {
super(scope, id);
defaults.CheckProps(props);
defaults.CheckSqsProps(props);

// Setup the dead letter queue, if applicable
this.deadLetterQueue = defaults.buildDeadLetterQueue(this, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import * as cdk from 'aws-cdk-lib';
import { EventbridgeToSqs, EventbridgeToSqsProps } from '../lib';
import * as events from "aws-cdk-lib/aws-events";
import * as sqs from "aws-cdk-lib/aws-sqs";
import { Template } from 'aws-cdk-lib/assertions';
import * as defaults from '@aws-solutions-constructs/core';

Expand Down Expand Up @@ -535,4 +536,26 @@ test('Queue purging flag grants correct permissions', () => {
}
]
});
});
});

test('check that CheckSqsProps is being called', () => {
const stack = new cdk.Stack();

const props: EventbridgeToSqsProps = {
eventRuleProps: {
eventPattern: {
source: ['solutionsconstructs']
}
},
eventBusProps: { eventBusName: 'test' },
queueProps: {
removalPolicy: cdk.RemovalPolicy.DESTROY,
},
existingQueueObj: new sqs.Queue(stack, 'test', {})
};

const app = () => {
new EventbridgeToSqs(stack, 'test-eventbridge-sqs', props);
};
expect(app).toThrowError("Error - Either provide queueProps or existingQueueObj, but not both.\n");
});
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export class FargateToDynamoDB extends Construct {
defaults.CheckProps(props);
defaults.CheckFargateProps(props);
defaults.CheckDynamoDBProps(props);
defaults.CheckVpcProps(props);

// Other permissions for constructs are accepted as arrays, turning tablePermissions into
// an array to use the same validation function.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import { Template } from 'aws-cdk-lib/assertions';
import * as defaults from '@aws-solutions-constructs/core';
import * as cdk from "aws-cdk-lib";
import { FargateToDynamoDB } from "../lib";
import { FargateToDynamoDB, FargateToDynamoDBProps } from "../lib";
import * as dynamodb from 'aws-cdk-lib/aws-dynamodb';
import * as ecs from 'aws-cdk-lib/aws-ecs';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
Expand Down Expand Up @@ -713,4 +713,37 @@ test('test that DDB input args are getting checked', () => {
};

expect(app).toThrowError('Error - Either provide existingTableInterface or dynamoTableProps, but not both.\n');
});
});

test('Confirm that CheckVpcProps was called', () => {
const stack = new cdk.Stack();
const publicApi = true;
const clusterName = "custom-cluster-name";
const containerName = "custom-container-name";
const serviceName = "custom-service-name";
const familyName = "custom-family-name";

const props: FargateToDynamoDBProps = {
publicApi,
ecrRepositoryArn: defaults.fakeEcrRepoArn,
clusterProps: { clusterName },
containerDefinitionProps: { containerName },
fargateTaskDefinitionProps: { family: familyName },
fargateServiceProps: { serviceName },
dynamoTableProps: {
tableName: 'fake-name',
partitionKey: {
name: 'id',
type: dynamodb.AttributeType.STRING
},
},
existingVpc: defaults.getTestVpc(stack),
vpcProps: { },
};

const app = () => {
new FargateToDynamoDB(stack, 'test-construct', props);
};
// Assertion
expect(app).toThrowError('Error - Either provide an existingVpc or some combination of deployVpc and vpcProps, but not both.\n');
});
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export class FargateToEventbridge extends Construct {
super(scope, id);
defaults.CheckProps(props);
defaults.CheckFargateProps(props);
defaults.CheckVpcProps(props);

this.vpc = defaults.buildVpc(scope, {
existingVpc: props.existingVpc,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import { Template } from 'aws-cdk-lib/assertions';
import * as defaults from '@aws-solutions-constructs/core';
import * as cdk from "aws-cdk-lib";
import { FargateToEventbridge } from "../lib";
import { FargateToEventbridge, FargateToEventbridgeProps } from "../lib";
import * as events from 'aws-cdk-lib/aws-events';
import * as ecs from 'aws-cdk-lib/aws-ecs';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
Expand Down Expand Up @@ -372,4 +372,29 @@ function createFargateConstructWithNewResources(stack: cdk.Stack, publicApi: boo
eventBusName: 'custom-name'
}
});
}
}

test('Confirm that CheckVpcProps was called', () => {
const stack = new cdk.Stack();
const publicApi = true;

const props: FargateToEventbridgeProps = {
publicApi,
ecrRepositoryArn: defaults.fakeEcrRepoArn,
clusterProps: { clusterName },
containerDefinitionProps: { containerName },
fargateTaskDefinitionProps: { family: familyName },
fargateServiceProps: { serviceName },
eventBusProps: {
eventBusName: 'custom-name'
},
existingVpc: defaults.getTestVpc(stack),
vpcProps: { },
};

const app = () => {
new FargateToEventbridge(stack, 'test-construct', props);
};
// Assertion
expect(app).toThrowError('Error - Either provide an existingVpc or some combination of deployVpc and vpcProps, but not both.\n');
});
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ export class FargateToKinesisFirehose extends Construct {
super(scope, id);
defaults.CheckProps(props);
defaults.CheckFargateProps(props);
defaults.CheckVpcProps(props);

if (!props.existingKinesisFirehose.deliveryStreamName) {
throw new Error('existingKinesisFirehose must have a defined deliveryStreamName');
Expand Down
Loading

0 comments on commit 73172bb

Please sign in to comment.