Skip to content

cdk: add cdk.json schema for validation and IntelliSense #2078

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

Closed
wants to merge 2 commits into from

Conversation

tinovyatkin
Copy link
Contributor

@tinovyatkin tinovyatkin commented Sep 26, 2021

Problem

cdk.json file that contains defaults for AWS CDK project are badly/partially documented and often mystified, while in reality it maybe an useful way to optimised most of CDK project development experience.
Some references:

Solution

This PR adds to the extenstion contribution a JSON schema for cdk.json file that provides rich experience with editing cdk.json files with documentation, IntelliSense for enum values, automatic validation (such as banned properties in context), etc.

Integration test added for cdk.json validation via this scheme in VSCode.

License

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

@tinovyatkin tinovyatkin requested a review from a team as a code owner September 26, 2021 11:04
@tinovyatkin tinovyatkin force-pushed the feature/cdk-json-schema branch from 91f4b0c to 7f7d31e Compare September 26, 2021 11:05
@tinovyatkin tinovyatkin changed the title Feature/cdk-json-schema cdk: add cdk.json schema for validation and IntelliSense Sep 26, 2021
@tinovyatkin tinovyatkin force-pushed the feature/cdk-json-schema branch 3 times, most recently from 970c68e to aeba42d Compare September 26, 2021 18:09
@tinovyatkin tinovyatkin force-pushed the feature/cdk-json-schema branch from aeba42d to f627d7e Compare September 26, 2021 20:55
@@ -0,0 +1,233 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct long term place for this file should probably be in the CDK repo and maintained by the CDK team.

There is no forcing function for us to maintain it, nor are we in the loop when it gets out of date. This would also allow us to share the file with the JetBrains and VS IDEs as well.

Have you tried to submit this to them?

Copy link
Contributor Author

@tinovyatkin tinovyatkin Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is misunderstanding of what cdk.json is for CDK (what I referred above as mystified). CDK per se doesn't depend on cdk.json (we can have completely correct CDK project without any cdk.json - just need to specify app from command line).
So, cdk.json is all about storing default values in a specific place. You can also store any arbitrary values there, just for your own pleasure or for a company extension.
And this VSCode extension is about making work experience with CDK better, so, this JSON schema helps on that.
CDK is pretty old project for now and if they didn't made the schema yet that means they don't care about editing/validating experience and left that to a third-party extensions. That's why I'm here.

Do you think this addition will be useful for users of this extensions? I'm 100% sure in that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tinovyatkin thank you for this! It seems useful for users beyond VSCode.

Before rushing to merge this PR, we would like to see if we can find an upstream home for this schema. I left a comment in aws/aws-cdk#12799

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @justinmk3, your answer sounds like from a customer obsessed company employee. I'll be happy to land the JSON schema in a different place, but ultimate contribution point for VSCode users (and so, for the unit test) will be this extension. So, we may considering merge in the meanwhile (to make customers happy) and if an upstream owner arise then just to change reference to the schema.

Copy link

@arnulfojr arnulfojr Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To have the most impact imho, the best place to put this schema would be to update the existing one in SchemaStore. After submission, most customers will get this out-of-the-box 🚀

https://www.schemastore.org/json/

https://github.com/SchemaStore/schemastore/blob/master/src/schemas/json/cdk.json

https://code.visualstudio.com/docs/languages/json#_json-schemas-and-settings
https://www.jetbrains.com/help/idea/json.html#ws_json_using_schemas

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnulfojr thanks for mentioning that! Your comment would be helpful at aws/aws-cdk#12799 also :)

We will look at leveraging schemastore as source of truth for AWS Toolkit (or does vscode do it automatically?)

@justinmk3
Copy link
Contributor

@tinovyatkin Just a note, I am still working with CDK team to see if this can be upstreamed.

@justinmk3
Copy link
Contributor

@tinovyatkin would you consider updating the schemastore.org definition at https://github.com/SchemaStore/schemastore/blob/master/src/schemas/json/cdk.json ?

How would this PR look if the schemastore cdk.json definition were used instead?

Comment on lines +7 to +16
"app": {
"description": "command-line for executing your app or a cloud assembly directory",
"examples": [
"node bin/my-app.js",
"npx ts-node --prefer-ts-exts bin/cdk-app.ts",
"node --abort-on-uncaught-exception --enable-source-maps --unhandled-rejections=strict --experimental-specifier-resolution=node --experimental-json-modules --loader ts-node/esm lib/app.ts"
],
"type": "string",
"minLength": 1
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wish cdk allow apps: string[] or app: string | string[] in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants