fix(standardize encryption props) Standardize how encryption properties are used for SNS/SQS constructs. #846
Conversation
georgebearden
requested review from
biffgaut,
hayesry,
mobri2a,
emcfins and
mickychetta
as code owners
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
As I read the unit tests for each construct, I got an impression that some only added 1 test while most added about 4. So I went and checked the path coverage for the affected constructs. While most were 100%, I also found:
Eventbridge-sqs 75%
Lambda-sns 90%
Lambda-sqs 77.7%
Can we check if the paths that we may have added are covered? Especially for the two in the seventies. (if you see a good way to get these in the 80's regardless of the reason, that would be great).
| */ | ||
| readonly enableEncryptionWithCustomerManagedKey?: boolean; | ||
| readonly enableEncryptionWithCustomerManagedKey?: boolean |
There was a problem hiding this comment.
Why are we dropping semicolons?
It makes us inconsistent across the library. I realize the Javascript (so therefore probably Typescript) provides "Automatic Semicolon Insertion" in "certain situations." Rather than depend upon everyone memorizing these "certain situations" we'll use semicolons everywhere.
There was a problem hiding this comment.
The identified constructs above (eventbridge-sqs, lambda-sns, lambda-sqs) have updated tests and are now at 100% coverage.
source/patterns/@aws-solutions-constructs/aws-eventbridge-sqs/lib/index.ts
Outdated
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| if (props.deployVpc && props.existingVpc) { | ||
| throw new Error("More than 1 VPC specified in the properties"); | ||
| } |
There was a problem hiding this comment.
Deleted because this check is redundant and can never be hit. This check is already being done from the defaults.CheckProps call here
| if (props.deployVpc && props.existingVpc) { | ||
| throw new Error("More than 1 VPC specified in the properties"); | ||
| } |
There was a problem hiding this comment.
Deleted because this check is redundant and can never be hit. This check is already being done from the defaults.CheckProps call here****
| expect(app).toThrowError('Error - Either provide an existingVpc or some combination of deployVpc and vpcProps, but not both.'); | ||
| }); | ||
|
|
||
| test('Queue purging flag grants correct permissions', () => { |
There was a problem hiding this comment.
@biffgaut Let's follow up on an observation I made regarding this test.
This PR standardizes how encryption properties are used across all constructs that utilize SQS Queues or SNS Topics. The core
sns-helperandsqs-helperclasses have been updated to be aware of encryption properties of the underlyingqueuePropsandtopicPropsproperties respectively. This PR passes all existing tests, and includes new tests to cover the various ways encryption can be set.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.