feat: Add FirehoseToS3Stage and tests #151

jaidisido · 2022-08-19T13:59:53Z

Add utils and defaults modules. Helper methods for the first, defaults for resources for the second
Add FirehoseToS3Stage
Add tests for aforementioned stage (100% coverage) and modify basic pipeline to include this stage instead

jaidisido

Clarifications

.projenrc.js

jaidisido · 2022-08-19T14:01:32Z

.projenrc.js

  ],
 });

+// Experimental modules


Might want to discuss how we wish to handle additional deps in .projenrc.js

jaidisido · 2022-08-19T14:02:55Z

src/core/kinesis-firehose-defaults.ts

@@ -0,0 +1,11 @@
+import { Compression, S3BucketProps } from '@aws-cdk/aws-kinesisfirehose-destinations-alpha';


This is how defaults are handled in the construct-hub library, what do we think about this approach? I like how it enables us to centralise these defaults instead of having them all over the place at the moment

I'm a fan, this will make precedence of settings (config, explicit props, defaults) a lot more readable in the stages as well.

I wouldn't create a separate file for the defaults tbh but otherwise looks good to me

jaidisido · 2022-08-19T14:03:34Z

src/core/utils.ts

+  return target;
+}
+
+export function overrideProps(DefaultProps: object, userProps: object, concatArray: boolean = false): any {


Some useful util functions to manipulate props (i.e. override and/or combine them)

Would be nice to have some documentation for them - they do look useful but some are far less obvious than others

jaidisido · 2022-08-19T14:05:25Z

src/stages/kinesis-s3.ts

+
+export interface FirehoseToS3StageProps extends DataStageProps {
+  readonly s3Bucket?: s3.IBucket;
+  readonly s3BucketProps?: s3.BucketProps;


I have used the existing BucketProps and destinations.S3BucketProps interfaces instead of defining my own. Is there are reason why we have adopted a different strategy in other stages (i.e. defining our own interfaces for these)?

Mostly I was copying these definitions as they were in Python. I figured the reasoning was that we didn't want them messing with some important defaults (like making a function fully open to the public or something). Or if they really did, they would have to pass their own Function/Bucket.

Another concern I have is that when a user reads the documentation for what goes into FirehoseToS3StageProps, they'll look at the properties of s3.BucketProps and then they'll read that if they don't set the encryption, it will be set to unencrypted. This will be confusing if we're offering different default values, such as S3_MANAGED for encryption. A benefit of defining our own set of properties is that at least our documentation will be accurate.

Fair points. I suppose my concern about restricting the arguments of Props is that there will always be a user who needs a specific argument that we wouldn't be supporting (c.f. Tracing in the Lambda Function). I find the idea of asking them to create their own Lambda function to be a bad UX and kind of defeats the point of DDK a bit. I suppose it goes back to the discussion on props and escape hatches again...

Typescript does let us hack around this a little bit. We could have something like:

interface OurBucketProps extends Omit<s3.BucketProps, "encryption"> { /** Our custom documentation, and here we can mention what our default is. */ readonly encryption: s3.EncryptionOption; }

This way, OurBucketProps extends a version of the BucketProps that omits the encryption property. This allows us to redefine it ourself, with our own documentation, while still inheriting all other properties. And if we then decide to do the same thing with enforceSsl, we simply inherit from Omit<s3.BucketProps, 'encryption', 'enforceSsl'> , and define our own enforceSsl.

This solution would technically solve the problem for us, although my primary concern here is I'm not sure how well this would play with other languages as it gets translated with JSII.

A pattern that I've seen is to extend the type even if there are no additions to the type at the moment i.e.

interface OurBucketProps extends s3.BucketProps {}

This way we can still keep the code backward-compatible:

Add properties when required

Break the inheritance if we have to divert from the original definition of a property at some point without changing the type of what the user has to provide

Omit & other utility types are cool but I would avoid them unless absolutely necessary as they're a bit of a hack

The problem is that without Omit, JSII will not let you redefine a property, even to just rewrite the property documentation. When I tried, it said it would break compatibility with C# (and Java I believe).

…is-s3-stage

LeonLuttenberger · 2022-08-19T14:25:40Z

src/stages/kinesis-s3.ts

+
+export interface FirehoseToS3StageProps extends DataStageProps {
+  readonly s3Bucket?: s3.IBucket;
+  readonly s3BucketProps?: s3.BucketProps;


Mostly I was copying these definitions as they were in Python. I figured the reasoning was that we didn't want them messing with some important defaults (like making a function fully open to the public or something). Or if they really did, they would have to pass their own Function/Bucket.

Another concern I have is that when a user reads the documentation for what goes into FirehoseToS3StageProps, they'll look at the properties of s3.BucketProps and then they'll read that if they don't set the encryption, it will be set to unencrypted. This will be confusing if we're offering different default values, such as S3_MANAGED for encryption. A benefit of defining our own set of properties is that at least our documentation will be accurate.

LeonLuttenberger · 2022-08-19T14:34:48Z

src/core/kinesis-firehose-defaults.ts

+import { Compression, S3BucketProps } from '@aws-cdk/aws-kinesisfirehose-destinations-alpha';
+import { Duration, Size } from 'aws-cdk-lib';
+
+export function DefaultDestinationsS3BucketProps(_dataOutputPrefix?: string) {


Minor: Why is Default capitalized? I thought the convention in Typescript was that functions are camelCase rather than PascalCase. We'll also need to figure out how to enforce these conventions. I think this rule should be good. Obviously this can be a separate PR.

Yep, not sure why they are doing that. Perhaps because it's supposed to mimick a Props interface. But I agree, I will switch to lower case

LeonLuttenberger · 2022-08-19T14:35:49Z

src/core/s3-defaults.ts

+    removalPolicy: RemovalPolicy.RETAIN,
+    enforceSSL: true,
+    eventBridgeEnabled: false,
+    ...((lifecycleRules !== undefined) && { lifecycleRules }),


What does this syntax do exactly? Does it check if lifecycleRules is set, and if it is, it puts it in the props?

That is my understanding indeed. If lifecycle rules is passed, add it to the object, otherwise ignore

This is very cool, I've been missing that in Python a LOT

LeonLuttenberger · 2022-08-19T14:46:34Z

src/stages/kinesis-s3.ts

+      throw TypeError("'s3Bucket' or 's3BucketProps' must be set to instantiate this stage");
+    }
+
+    const dataStreamEnabled = props.dataStreamEnabled ?? false;


Could we have a function that assigns default, in order to avoid having to do it this way? Or are we only using that structure for original CDK constructs?

I felt that creating a function to handle defaults for a single variable to be a bit of an overkill as it could quickly get out of hands

…is-s3-stage

kukushking

LGTM! Some ongoing discussions in threads but overall looks great!

…is-s3-stage

(feat): Add FirehoseToS3Stage and tests

2bc9c50

jaidisido commented Aug 19, 2022

View reviewed changes

jaidisido requested review from LeonLuttenberger, kukushking, cnfait and malachi-constant August 19, 2022 14:05

jaidisido self-assigned this Aug 19, 2022

jaidisido added feature-request effort/medium labels Aug 19, 2022

(deps): Upgrade to CDK version 2.38.1

183adae

jaidisido changed the base branch from typescript-conversion to main August 19, 2022 14:25

jaidisido changed the base branch from main to typescript-conversion August 19, 2022 14:25

jaidisido force-pushed the typescript-conversion-kinesis-s3-stage branch from 90733f5 to 183adae Compare August 19, 2022 14:28

Merge branch 'typescript-conversion' into typescript-conversion-kines…

c0161ca

…is-s3-stage

jaidisido changed the title ~~(feat): Add FirehoseToS3Stage and tests~~ feat: Add FirehoseToS3Stage and tests Aug 19, 2022

LeonLuttenberger reviewed Aug 19, 2022

View reviewed changes

jaidisido added this to In Review in Roadmap Aug 19, 2022

jaidisido added 2 commits August 22, 2022 11:31

Merge branch 'typescript-conversion' into typescript-conversion-kines…

cc955e3

…is-s3-stage

PR feedback, bundled deepmerge dep, and prettier

778bf06

kukushking approved these changes Aug 22, 2022

View reviewed changes

Switch to require import for deepmerge

b1613af

jaidisido added this to the 1.0.0 milestone Aug 22, 2022

jaidisido added 2 commits August 23, 2022 11:09

Merge branch 'typescript-conversion' into typescript-conversion-kines…

f595474

…is-s3-stage

Fix API.md

cbd3171

jaidisido merged commit 03216e5 into typescript-conversion Aug 23, 2022

jaidisido deleted the typescript-conversion-kinesis-s3-stage branch August 23, 2022 10:36

malachi-constant moved this from In Review to Done in Roadmap Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add FirehoseToS3Stage and tests #151

feat: Add FirehoseToS3Stage and tests #151

jaidisido commented Aug 19, 2022

jaidisido left a comment

jaidisido Aug 19, 2022

jaidisido Aug 19, 2022

malachi-constant Aug 19, 2022

kukushking Aug 22, 2022

jaidisido Aug 19, 2022

kukushking Aug 22, 2022

jaidisido Aug 19, 2022

LeonLuttenberger Aug 19, 2022

jaidisido Aug 19, 2022

LeonLuttenberger Aug 19, 2022

kukushking Aug 22, 2022

kukushking Aug 22, 2022

LeonLuttenberger Aug 23, 2022

LeonLuttenberger Aug 19, 2022

LeonLuttenberger Aug 19, 2022

jaidisido Aug 19, 2022

LeonLuttenberger Aug 19, 2022

jaidisido Aug 19, 2022

kukushking Aug 22, 2022

LeonLuttenberger Aug 19, 2022

jaidisido Aug 19, 2022

kukushking left a comment •

edited

		@@ -0,0 +1,11 @@
		import { Compression, S3BucketProps } from '@aws-cdk/aws-kinesisfirehose-destinations-alpha';

feat: Add FirehoseToS3Stage and tests #151

feat: Add FirehoseToS3Stage and tests #151

Conversation

jaidisido commented Aug 19, 2022

jaidisido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kukushking left a comment • edited

Choose a reason for hiding this comment

kukushking left a comment •

edited