Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC655: Enhanced L1s #657
base: main
Are you sure you want to change the base?
RFC655: Enhanced L1s #657
Changes from 5 commits
a38a446
bd20c7a
cba818d
b572300
9a2d7b6
3ebc7de
06529f4
094791a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does
cfn-lint
get the data from? Any reason not to include this data source directly?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cfn-lint
already has a process built that pulls the data from the AWS SDK (specifically boto3). Idea is to not re-implement that ourselves. CFN Lint has similar data requirements/usage, so (imo) makes more sense to re-use. Ideally this data should just be in the CFN schemas per service directly, which is something we can work withcfn-lint
and CFN on directly. That is outside of this, but helps with the full picture.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK AWS CLI is based on the Smithy service definitions. Would it make sense to pull these as a data source and cross reference with the CFN service spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is (more or less) what
cfn-lint
is doing (see above comment) but packaging it as CFN schemas that we can just leverage instead of doing ourselves.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a huge help. It's immensely frustrating to have so many checks in your CDK code only to have a simple validation check fail in Cloudformation. Thank you for recommending this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's enough "meat" for CDKv3, then I would recommend choosing this approach, but bear in mind that a major version is a big project (v2 tool us almost two years to release).
It is also okay to punt this particular enhancement until you are ready to release v3 given its breaking nature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the more thorough validations added with the enhanced L1s!
One question, how does this work when the original prop is required? For example in
CfnTableProps
, thekeySchema
property is required, something like below:After enhancing L1, we cannot make both original and v2 prop required, because users only need to set one.
So I think we have to make the interface something like this:
But imho this is actually worse than the original L1, because it lost the information that keySchema is a required property. (of course we can do runtime validation, but type check is faster.)
This is another reason why I'm not a big fan of option 1. I like option 3 better with the upgrade of aws-cdk-lib to v3 (and possibly aws-cdk v3) as @johnf said. With cdk v3, we can also remove all the feature flags that have been added since the v2 release, which has been a burden for construct library maintainers to validate their code with various possible flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good example, my thought was we could do runtime validation (making sure one of these is set) but as you mentioned type check is faster. I'll add this example to the cons for option 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn this stuff is hard!
For option 1, I agree that the loss of strong typing (turning required properties to optional) is a major drawback.
But option 1 is tricky for another reason: you will likely want to deprecate the legacy properties and keep only the modern versions. In the above example, it means that you will need to delete
keySchema
and only leavekeySchema_v2
which is not a good outcome. If you end up modernizingkeySchema
, you'll need to removekeySchema_v2
and then you will penalize the users who migrated to the modern version earlier by breaking or deprecating their code.I would say the best approach would be to go with option 3 and couple this feature with a new major version (v3) of the CDK, which will allow you to break the existing APIs. When we planned v2 we had a GitHub issue where we accumulated these major breaking changes, and at some point we got to a critical mass that justified a new major version.
If v3 is too far away and there's a very strong push to introduce this capabilities sooner, I think option 2 is a reasonable tradeoff albeit there's a chance it will cause the library to be too large.
I am wondering if perhaps as an intermediate step (before a major version is released), you can just add better documentation and runtime (synth-time) checks that the value is valid. This will go a long way to shift-left errors without breaking. In TypeScript, you could even use a type union like
"foo" | "bar" | "baz"
to improve typings without impacting JSII languages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By modernizing
keySchema
do you mean if/when we go to CDK V3 we'd have to go back tokeySchema
?Updating the docs is a great idea and the implementation should be fairly straight forward. For the runtime validation, is it possible to do without introducing a new property and without breaking compatibility?
Do you have a link to this github issue? Couldn't find it through a quick search 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In the long run that's the "right" name.
I believe it should be possible to only add runtime validations instead of introducing new types.
https://github.com/aws/aws-cdk-rfcs/blob/main/text/0079-cdk-2.0.md
That's the relevant RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a better way to manage the backwards compatibility issues here, but I'm not crazy about the possibility of having
runtime_v2
defined but notruntime
. Can't think of specific examples but I get the impression validation becomes a little more difficult - if you must have a property defined, now you have to account for that property and its _v2 variant, which could introduce issues for certain use cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if perhaps it would be sufficient to just add
fromCfnXxx
to all L2s instead of juggling all of these interfaces.These conversion static methods would be straightforward to implement:
Indeed, it won't allow users to directly pass an L1 everywhere an
IBucket
is needed but it feels like this would be good enough to improve the ergonomics without too many breaking changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bucket L2 already has a
fromCfnBucket
that implements what you mentioned but I'm not sure how many other L2s have something similar or what the effort to add the missing ones would be. Could be worthwhile alternative but I don't think it fixes the root issue (and it becomes another thing we have to manually make rather than having it be auto generated :/ )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be easy to enforce the
fromCfnXxx
static methods using awslint so it's semi-manual.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cautiously optimistic about this approach, and definitely curious about the findings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can look at this branch for a deeper look on this POC: https://github.com/aws/aws-cdk/tree/conroy/generate