-
Notifications
You must be signed in to change notification settings - Fork 249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(aws-sns-sqs): New aws-sns-sqs pattern implementation #48
feat(aws-sns-sqs): New aws-sns-sqs pattern implementation #48
Conversation
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 |
… latest cdk construct version 1.60.0
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…a KMS key with rotation to perform SQS encryption.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good first cut, apart from some minor updates, suggest to expose encryptionKeyProps?
via construct props
* @param {cdk.App} scope - represents the scope for all the resources. | ||
* @param {string} id - this is a a scope-unique id. | ||
* @param {SnsToSqsProps} props - user provided props for the construct. | ||
* @since 0.8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@SInCE 1.60.0" ?
}); | ||
} | ||
|
||
let enableEncryptionParam = props.enableEncryption; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declare variable type enableEncryptionParam:boolean
} | ||
|
||
let enableEncryptionParam = props.enableEncryption; | ||
let encryptionKeyParam = props.encryptionKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declare variable type encryptionKeyParam:kms.Key
* | ||
* @default - not specified. | ||
*/ | ||
readonly encryptionKey?: kms.Key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more optional argument for user-provided encryptionKeyProps? readonly encryptionKeyProps?: kms.KeyProps
if (props.enableEncryption !== false) { | ||
enableEncryptionParam = true; | ||
if (!props.encryptionKey) { | ||
encryptionKeyParam = buildEncryptionKey(scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the user provided props.encryptionKeyProps
to override
import '@aws-cdk/assert/jest'; | ||
|
||
// -------------------------------------------------------------- | ||
// Pattern deployment w/ new Lambda function and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Lambda function
?
// Initial Setup | ||
const stack = new Stack(); | ||
const props: SnsToSqsProps = { | ||
topicProps: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it omitted since its empty anyway ?
enableEncryption: true, | ||
deployDeadLetterQueue: true, | ||
maxReceiveCount: 0, | ||
queueProps: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it omitted since its empty anyway ?
let enableEncryptionParam = props.enableEncryption; | ||
let encryptionKeyParam = props.encryptionKey; | ||
// Create the encryptionKey if none was provided | ||
if (props.enableEncryption !== false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it require a check for === undefined
as well ? If user does not provide this optional parameter
* | ||
* @default - false (encryption enabled with a KMS key managed by SQS). | ||
*/ | ||
readonly enableEncryption?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this parameter be renamed to indicate its required only for Customer Managed KMS Key ? e.g. enableEncryptionWithCustomerManagedKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more minor update
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
Thanks for the contribution! It will be pushed out in the next release of Constructs library! |
closes #24
Issue #, if available: #24
Description of changes:
Implements the AWS Solutions Construct that creates an Amazon SNS Topic connected to an Amazon SQS queue.
In addition, it includes the changes in the sqs-helper.ts to support an imported KMS customer managed CMK.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.