aws-cdk-rfcs icon indicating copy to clipboard operation
aws-cdk-rfcs copied to clipboard

RFC 655: Enhanced L1s

Open aaythapa opened this issue 1 year ago • 6 comments

This is a request for comments about enhancing L1s in CDK. See #655 for additional details.

APIs are signed off by @alias.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aaythapa avatar Nov 18 '24 23:11 aaythapa

I love everything being proposed here. This will bring significant improvements!

Except for the _v2 suggestion. I understand why this approach is being suggested, but it doesn't feel right to me.

I think this adds cognitive load to the developer, I have to now think about wether to use _v2 or not. Also, does this stay around forever, or does it get dumped in a future change? DO we end up with _v3 down the track?

My suggestion: Bite the bullet, make a breaking change and bump the major version.

A possible approach to help, if we release today

  • release aws-cdk-lib 3.174.0 and 2.174.0.
  • Keep the versions in sync
  • The only difference would be v3 has the prop_v2 logic written out to prop
  • So this should be automatable

Some alternatives I can think of

  • Make a breaking change and release a codemod to help everyone upgrade their code
  • Typescript magic: pass in something like cdk_type_version: 2 into every prop, which gives me the new types - in the future, we could make this go away. I suspect there is some Typescript magic that might allow us to set something globally and have this still work

johnf avatar Dec 18 '24 20:12 johnf

I love the idea of having Interfaces for the CFN L1s and improving the L1-L2 interoperability.

I agree with @johnf, it has been a few years since a major release and there are many Flags in cdk.json and @deprecated functions. Including this big change, it justifies a new major release for me.

rehanvdm avatar Dec 19 '24 06:12 rehanvdm

At re:Invent 2024 I had the opportunity to discuss this with a number of AWS Heroes: @hoegertn @Osterjour @pgarbe This is a write-up of this discussion from my perspective. Please add to it as required.

It was questioned if enums are necessary in L1s as they don't seem to add much value and take L1s further away from what the CFN is. It was also noted that the use of _v2 is confusing and annoying. In particular deploying this pattern for enums will make it a regular occurrence across the L1 library, as opposed to using the suffix only in situations when an upstream type change is breaking. In response to this, it was noted that old properties would be marked as deprecated and that it is an IDE and documentation concern to ensure that users are not flooded with multiple of the same props to choose from.

The discussion then dived into what L1s are representing, what they should be representing and what they could be representing in future. There was consensus that L1 are currently roughly equal to CFN, however it was not clear what that means exactly. As an example, Validations were brought up which are currently not part of L1s but having them would make them arguably closer to what CFN does. Another example was IAM grants. While the group agreed that this is not currently part of CFN, it would be very useful if grants can be auto-generated from a data source.

Based on this, the idea was proposed that L1s could be everything that can be auto generated (i.e. not hand-written) from a data source. It was suggested that maybe this should be separate layer and the proposed features in the RFC plus future features like grants could be introduced as a new layer between the current L1 and L2. However concerns were raised that this was also confusing.

End of the discussion.

mrgrain avatar Dec 19 '24 09:12 mrgrain

This is an excellent suggestion. It would be especially nice to have the built-in validation in L1 by default.

And most of my opinions have already been mentioned above. Introducing V2 for each parameter could lead to complexity (props become bloated, required properties become optional, etc.).

Also, many users have created a mechanism to check against L1 resource properties using Aspects, and I'm concerned that the addition of this v2 property might break that (existing) mechanism.

Therefore, I agree with the opinion to upgrade with CDK V3 (or aws-cdk-lib-v2 mentioned as option.3) as they say.

go-to-k avatar Dec 20 '24 04:12 go-to-k

Thank you all for the feedback everyone!

The general consensus seems to be that the features are valuable and would make a great addition to the L1s. However, the proposed implementation (_v2 suffix on the new properties), compromises UX while aiming to maintain backward compatibility. Most would prefer moving forward with CDK V3 instead.

I'll share this feedback with the team and provide updates here as they come. In the meantime, please feel free to share any additional comments or suggestions. Thanks again

aaythapa avatar Dec 23 '24 23:12 aaythapa

It was questioned if enums are necessary in L1s as they don't seem to add much value and take L1s further away from what the CFN is. It was also noted that the use of _v2 is confusing and annoying. In particular deploying this pattern for enums will make it a regular occurrence across the L1 library, as opposed to using the suffix only in situations when an upstream type change is breaking.

I may have missed the point here, but to me, having enums where there is a defined list of possible values, is very valuable. Its an almost every day occurrence having to look at the docs, to check on valid values.

Why would this not be valuable?

mrpackethead avatar Jan 05 '25 07:01 mrpackethead