feat(aws-events-rule-kinesisfirehose-s3): added logS3AccessLogs and loggingBucketProps #492
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 |
| @@ -1,90 +1,6 @@ | |||
| { | |||
| "Description": "Integration Test for aws-eventbridge-kinesisfirehose-s3", | |||
| "Resources": { | |||
| "testeventbridgekinesisfirehoses3KinesisFirehoseToS3S3LoggingBucket703E6C44": { | |||
There was a problem hiding this comment.
This looks odd - a no arguments test should still create a logging bucket. Isn't logging on by default? So no args should have logging (and had logging before any change we made in this PR).
| bucketProps: { | ||
| removalPolicy: RemovalPolicy.DESTROY | ||
| }, | ||
| logS3AccessLogs: false |
There was a problem hiding this comment.
Ah - this is driving the change I saw in the expected.json file. How does this align with the name "no-args"
There was a problem hiding this comment.
Should be no-arguments, will update this
| @@ -68,6 +68,19 @@ export interface EventbridgeToKinesisFirehoseToS3Props { | |||
| * @default - Default props are used | |||
| */ | |||
| readonly logGroupProps?: logs.LogGroupProps; | |||
| /** | |||
There was a problem hiding this comment.
It looks like you've made the same edits to this file in this PR and in aws-eventbridge-kinesisfirehose-s3. That's going to get pretty messy. If we merge this PR will it make 491 unnecessary?
There was a problem hiding this comment.
Yes, let's marge this pull request since it has all the commits in 491 and new changes. We will go ahead and close 491 after 492 merges
| logGroupProps: { | ||
| removalPolicy: RemovalPolicy.DESTROY | ||
| }, | ||
| logS3AccessLogs: false |
There was a problem hiding this comment.
Why are we turning off Access logs entirely for our integration tests?
There was a problem hiding this comment.
To fully clean up our buckets you mentioned to either turn off logging or add removalPolicy for our tests. Turning off logging would prevent creating additional integ.noLoggingBucket.ts file since it would already be tested in the other integ tests. Do you think it would be better to have integ.noLoggingBucket.ts and leave logging on or just integrate it with current tests?
| removalPolicy: RemovalPolicy.DESTROY | ||
| }, | ||
| logGroupProps: { | ||
| removalPolicy: RemovalPolicy.DESTROY | ||
| } |
There was a problem hiding this comment.
Interesting, this no arguments test leaves the logging turned on...
OTOH - we don't send logging bucket props to the logging bucket is not cleaned up as far as I can see.
There was a problem hiding this comment.
Specifying a removalPolicy for bucketProps and not loggingBucketProps will copy the same removalPolicy over to loggingBucketProps because of our condition
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 #485 , if available:
Description of changes:
-Added logS3AccessLogs and LoggingBucket props
-Created integ and unit tests for logging bucket
-Added pattern prop S3BucketInterface
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.