-
Notifications
You must be signed in to change notification settings - Fork 248
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-events-rule-sqs & aws-events-rule-sns): New aws-events-rule-sqs and aws-events-rule-sns pattern implementation #55
Conversation
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, lots of minor comments, but overall looks good, request to add one more integ test case for both patterns to show the usage of existingTopicObj?
or existingQueueObj?
source/patterns/@aws-solutions-constructs/aws-events-rule-sns/README.md
Outdated
Show resolved
Hide resolved
} | ||
}; | ||
|
||
new EventsRuleToSNSTopic(stack, 'test-events-rule-sns', props); |
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 this
instead of stack
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 stack is a variable and the module does not include a class which extends a construct this wont work here. Let me know if I am missing anything I followed the other integration test patterns?
source/patterns/@aws-solutions-constructs/aws-events-rule-sns/README.md
Outdated
Show resolved
Hide resolved
source/patterns/@aws-solutions-constructs/aws-events-rule-sns/README.md
Outdated
Show resolved
Hide resolved
|
||
| **Name** | **Type** | **Description** | | ||
|:-------------|:----------------|-----------------| | ||
|sqsQueueProps?|[`sqs.QueueProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-sqs.QueueProps.html)|User provided props to override the default props for the SQS Queue. | |
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 queueProps
instead of sqsQueueProps
.. Check other -sqs- patterns for reference
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.
Modified the files to queueProps.
| **Name** | **Type** | **Description** | | ||
|:-------------|:----------------|-----------------| | ||
|sqsQueueProps?|[`sqs.QueueProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-sqs.QueueProps.html)|User provided props to override the default props for the SQS Queue. | | ||
|eventRuleProps|[`events.RuleProps`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-events.RuleProps.html)|User provided eventRuleProps to override the defaults. | |
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.
Add existingQueueObj?
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.
Added the existingQueueObj to the documentation.
### Amazon CloudWatch Events Rule | ||
* Grant least privilege permissions to CloudWatch Events to publish to the SQS Queue | ||
|
||
## Architecture |
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.
Default settings for sqs.Queue
are missing
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.
Added the default settings documentation for sqs.Queue.
super(scope, id); | ||
|
||
// Setup the dead letter queue, if applicable | ||
if (props.deployDeadLetterQueue || props.deployDeadLetterQueue === undefined) { |
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.
Check if the user has provided existingQueueObj?
then skip, also add an integ test case for this scenario if user provides an existingQueueObj?
and no value for deployDeadLetterQueue?
it should not deploy the DLQ.. this might be an issue with the other existing -sqs- patterns as well..
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.
Modified the function to test if the existingQueueObj is being passed and in case its passed deadletter queue is not created, also added an integration test case to verify this scenario.
}) | ||
|
||
}) | ||
|
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.
Add a test case to check for events rule permissions, check here for reference
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.
Included an additional test case for events rule permissions.
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!
*Issue #42 New Pattern: aws-events-rule-sns
*Issue #25 New Pattern: aws-events-rule-sqs
Description of changes:
Adding two new constructs aws-events-rule-sns and aws-events-rule-sqs.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.