feat(aws-kinesisfirehose): DeliveryStream API and basic S3 destination #15544
Conversation
|
|
madeline-k
requested review from
BenChaimberg,
otaviomacedo,
rix0rrr,
skinny85 and
a team
packages/@aws-cdk/aws-kinesisfirehose-destinations/test/integ.s3-basic.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-kinesisfirehose-destinations/package.json
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-kinesisfirehose-destinations/package.json
Outdated
Show resolved
Hide resolved
| deliveryStreamType: 'DirectPut', | ||
| ...destinationConfig.properties, | ||
| }); | ||
| resource.node.addDependency(this.grantPrincipal); |
There was a problem hiding this comment.
This is also probably not what you want. You're probably looking to take a dependency on a particular grant, not on all grants every done to this principal. The grants have an API for doing exactly that.
…roperty type and use local var instead
…kinesisfirehose-deliverystream
There was a problem hiding this comment.
Looks absolutely fantastic 🔥. Amazing work @BenChaimberg @madeline-k and @otaviomacedo!
A few minor optional comments if you're interested.
packages/@aws-cdk/aws-kinesisfirehose-destinations/lib/private/index.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-kinesisfirehose-destinations/lib/s3-bucket.ts
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| it('creates a role when none is provided', () => { | ||
| const capturedRoleArn = Capture.aString(); |
There was a problem hiding this comment.
I would actually put the logical ID of the Role directly in the test. I understand it will be an ugly, hashed logical ID. But I think that's good: in general, you want the tests to act as a guard to make sure the logical IDs of elements in the template you generate are stable while you're doing refactorings (it's easy to change them when changing the construct tree when refactoring).
I understand changing this ID in theory would not be a breaking change, because Roles are stateless, but in my opinion you should be forced to update the unit tests when that happens (in other words, this should be a conscious decision, not something that happens accidentally).
Same comment applies to the tests below.
Co-authored-by: Adam Ruka <[email protected]>
madeline-k
added
contribution/core
BenChaimberg
removed
the
pr/do-not-merge
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
aws#15544) This PR implements the minimum DeliveryStream API and S3 destination. More features for DeliveryStream and the S3 destination will follow in future PRs. This work is being tracked in https://github.com/aws/aws-cdk/milestone/16 For more context, see: aws#15505 and the RFC: aws/aws-cdk-rfcs#342 closes aws#10810, aws#15499 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws#15544) This PR implements the minimum DeliveryStream API and S3 destination. More features for DeliveryStream and the S3 destination will follow in future PRs. This work is being tracked in https://github.com/aws/aws-cdk/milestone/16 For more context, see: aws#15505 and the RFC: aws/aws-cdk-rfcs#342 closes aws#10810, aws#15499 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR implements the minimum DeliveryStream API and S3 destination.
More features for DeliveryStream and the S3 destination will follow in future PRs. This work is being tracked in https://github.com/aws/aws-cdk/milestone/16
For more context, see: #15505 and the RFC: aws/aws-cdk-rfcs#342
closes #10810, #15499
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license