feat(aws-fargate-sqs): Created new construct #588
Conversation
mickychetta
requested review from
biffgaut,
hayesry and
hnishar
as code owners
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
source/patterns/@aws-solutions-constructs/aws-fargate-sqs/README.md
Outdated
Show resolved
Hide resolved
| const stack = new cdk.Stack(undefined, undefined, { | ||
| env: { account: "123456789012", region: 'us-east-1' }, | ||
| }); | ||
| const publicApi = true; |
There was a problem hiding this comment.
Is publicApi = true supposed to configure VPC in certain way? If so, how is it asserted in the unit tests below for publicApi = true vs false?
There was a problem hiding this comment.
Yes publicApi is a required construct prop which determine the type of subnets to create.
publicApi = true
// Confirm we created a Public/Private VPC
expect(stack).toHaveResourceLike('AWS::EC2::InternetGateway', {});
expect(stack).toCountResources('AWS::EC2::VPC', 1);
expect(stack).toCountResources('AWS::ECS::Service', 1);
publicApi = false
// Confirm we created an Isolated VPC
expect(stack).not.toHaveResourceLike('AWS::EC2::InternetGateway', {});
expect(stack).toCountResources('AWS::EC2::VPC', 1);
expect(stack).toCountResources('AWS::ECS::Service', 1);
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
|
||
| // Enable message send and receive permissions for Fargate service by default | ||
| this.sqsQueue.grantSendMessages(this.service.taskDefinition.taskRole); | ||
| this.sqsQueue.grantConsumeMessages(this.service.taskDefinition.taskRole); |
There was a problem hiding this comment.
Interesting - lambda-sqs only calls grantSendMessages(). Looking at what you do here it seems that perhaps we should add grantConsumeMessages to that construct. Either way, they should be consistent. I can see arguments either way - let's get the team together to discuss. Did you copy grantConsumeMessages from an SQS based construct that already exists?
There was a problem hiding this comment.
Did not copy grantConsumeMessages from an existing construct but thinking of different use cases, we should allow users to read and write to the queue
source/patterns/@aws-solutions-constructs/aws-fargate-sqs/test/fargate-sqs.test.ts
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-fargate-sqs/test/fargate-sqs.test.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-fargate-sqs/test/fargate-sqs.test.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-fargate-sqs/test/fargate-sqs.test.ts
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-fargate-sqs/test/fargate-sqs.test.ts
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Let's add a prop:
|queuePermissions?|`string[]`|Optional queue permissions to grant to the Fargate service. One or more of the following may be specified: `Read`,`Write`. Default is 'Write' |
Chose Write for default as a service will be one or the other in most cases so I had to choose one. Since the preferred way to read a queue is a event driven Lambda function, I chose write - but I'm open to discussion.
(don't forget to check this functionality in a test)
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #587 , if available:
Description of changes:
-Created new construct with Fargate connected to SQS
-Added environment variables
-Able to create VPC, service, queue, and accept existing ones
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
fixes #587