T131 - Logging buckets #164
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
hnishar
suggested changes
Apr 26, 2021
| @@ -67,6 +67,10 @@ export class CloudFrontToS3 extends Construct { | |||
| super(scope, id); | |||
| let bucket: s3.Bucket; | |||
|
|
|||
| if (props.existingBucketObj && props.bucketProps) { | |||
There was a problem hiding this comment.
1 - Add a unit test case to assert this scenario
2 - Update the input props comments and README.md to reflect the new behavior, currently it implies that existingBucketObj takes precedence over bucketProps which no longer will be the case
Similar feedback for the rest of the updates in PR
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 |
hnishar
approved these changes
Apr 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue 165
Description of changes:
some integration tests created an existing bucket using our core/BuildBucket() function - this then created a logging bucket when it created the existingBucket, not within our construct.
Some tests passed the existing bucket into the construct, but also passed a bucketProps objects (with a removal policy). This confused the construct so it apparently created extra bucket(s) and left some behind after cleanup.
So the solution to the issue is to add a check to the construct to fail if both existing bucket and bucket props are provided and ensure the integration tests all send the proper combination of inputs.