Skip to content
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

[aws-kinesisfirehose-s3] Question on property type / Unable to use Kinesis data stream as a source #73

Closed
dscpinheiro opened this issue Sep 21, 2020 · 2 comments
Labels
bug Something isn't working

Comments

@dscpinheiro
Copy link
Contributor

I have two questions about the aws-kinesisfirehose-s3 pattern (please let me know if it makes more sense to split them into different issues):

  1. Is there a reason why kinesisFirehoseProps's type is kinesisfirehose.CfnDeliveryStreamProps | any? This is the same type definition as aws-iot-kinesisfirehose-s3, but not aws-kinesisfirehose-s3-and-kinesisanalytics.

readonly kinesisFirehoseProps?: kinesisfirehose.CfnDeliveryStreamProps | any;

Because of that, VSCode cannot show me the properties from CfnDeliveryStreamProps:

Screen Shot 2020-09-21 at 12 37 19 PM

  1. Kinesis Firehose supports two types (DirectPut and KinesisStreamAsSource). As of now, it isn't very straightforward to use KinesisStreamAsSource, as the configuration for it (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-kinesisfirehose-deliverystream-kinesisstreamsourceconfiguration.html) requires the customer to pass the ARN of the role that provides access to the source Kinesis data stream (which is not available until after the construct is created). Does it make sense to create a separate pattern (e.g. aws-kinesisstreams-kinesisfirehose-s3) or modify this one to handle both scenarios?

This is 🐛 Bug Report

@hnishar
Copy link
Contributor

hnishar commented Sep 22, 2020

dscpinheiro@,

  1. kinesisFirehoseProps's type is kinesisfirehose.CfnDeliveryStreamProps | any to allow for user to override the extendedS3DestinationConfiguration properties without requiring to specify the bucketArn which is created by the construct. For example, this test case will not even compile if | any is dropped. Probably the same should apply for aws-kinesisfirehose-s3-and-kinesisanalytics pattern as well.

  2. Agree, it makes sense to create a new pattern aws-kinesisstreams-kinesisfirehose-s3 but internally it should still make use of aws-kinesisfirehose-s3 pattern similar to aws-iot-kinesisfirehose-s3

@dscpinheiro
Copy link
Contributor Author

1. `kinesisFirehoseProps`'s type is `kinesisfirehose.CfnDeliveryStreamProps | any` to allow for user to override the `extendedS3DestinationConfiguration` properties without requiring to specify the `bucketArn` which is created by the construct. For example, this [test case](https://github.com/awslabs/aws-solutions-constructs/blob/master/source/patterns/%40aws-solutions-constructs/aws-kinesisfirehose-s3/test/test.kinesisfirehose-s3.test.ts#L82) will not even compile if ` | any` is dropped. Probably the same should apply for `aws-kinesisfirehose-s3-and-kinesisanalytics` pattern as well.

I get it, that makes sense. It's a bit unfortunate, but I can create the props separately to get intellisense.

const props: CfnDeliveryStreamProps = { ... };
new KinesisFirehoseToS3(this, 'KdfToS3', { kinesisFirehoseProps: props });

Hopefully this will improve when there's a L2 construct for Firehose.


2. Agree, it makes sense to create a new pattern `aws-kinesisstreams-kinesisfirehose-s3` but internally it should still make use of `aws-kinesisfirehose-s3` pattern similar to `aws-iot-kinesisfirehose-s3`

I'll create a separate ticket for this new pattern.

@hnishar hnishar added the bug Something isn't working label Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants