-
-
Notifications
You must be signed in to change notification settings - Fork 311
Add support for AWS CDK #1037
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
base: main
Are you sure you want to change the base?
Add support for AWS CDK #1037
Conversation
- Introduced a new plugin for AWS CDK with configuration options. - Added necessary package.json files for aws-cdk and its dependencies. - Implemented plugin logic in src/plugins/aws-cdk/index.ts and types.ts. - Updated schema to include aws-cdk plugin configuration. - Enhanced the main plugin index to register the aws-cdk plugin. - Created tests to validate dependency detection for the aws-cdk plugin.
Great start!
I see
The external dependencies can be added to
The |
currently the tests are reporting the following: bun test v1.2.9 (9a329c04)
test/plugins/aws-cdk.test.ts:
{
files: Set {},
dependencies: {},
devDependencies: {
"package.json": {
"aws-cdk": [Object ...],
},
},
optionalPeerDependencies: {},
unlisted: {
"cdk.json": {
cdk: [Object ...],
},
},
binaries: {},
unresolved: {},
exports: {},
nsExports: {},
types: {},
nsTypes: {},
enumMembers: {},
classMembers: {},
duplicates: {},
_files: {},
}
✓ Find dependencies with the aws-cdk plugin [97.81ms]
1 pass
0 fail
Ran 1 tests across 1 files. [589.00ms] |
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.
OK this is great progress, thanks for working on this!
| if (!config) return []; | ||
|
|
||
| const app = config.app.split(" ")[0]; | ||
| const watch = config.watch?.include ?? []; |
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.
My guess would be that the watch values aren't necessarily dependencies, nor entry files. Maybe we should ignore those. Maybe there are other options that represent entry points?
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.
To the best of my knowledge, the watch.include entries are file that should form the codebase for the IaC when using AWS CDK. From the name, it would indicate those are source files being watched for changes when running cdk watch which would allow an immediate changes of the generated CloudFormation template.
Those are not mandatory properties on the json Object, but should they be specified, they should be identical to the expected source files (bin/**/*.ts, lib/**/*.ts, cdk/**/*.ts. It is possible that some project are configured differently and those source files are living somewhere else. I would probably need help determining how to leave it to knip's users to specify this through configuration.
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 we would add the watch.include patterns, then they should be returned as project files (not entry files), i.e. toInput('bin/**/*.ts'). Adding "all" files as entry files would have two disadvantages: by default, unused exports of entry files are not reported, and Knip wouldn't be able to find unused files (as entry files are inherently used files).
However, the default project pattern is **/*.{js,ts}, so adding more specific patterns from a plugin wouldn't be effective.
My suggestion would be to ignore the watch.include patterns and see whether the defaults Knip finds are sufficient, then we could add default entry patterns in the plugin if there are any, and perhaps there are specific entry patterns to be found in aws-cdk configuration files.
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.
Ah ok that makes sense. I'll remove that 'watch' from the returned list.
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.
Addressed in 7efb03d
| const resolveConfig: ResolveConfig<AwsCdkConfig> = async config => { | ||
| if (!config) return []; | ||
|
|
||
| const app = config.app.split(" ")[0]; |
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.
The app option looks like a command, we should use getInputsFromScripts for this.
This function is available on the second options argument that ResolveConfig is invoked with.
Look for getInputsFromScripts in the plugins folder for other usage examples.
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.
Addressed in 7c45bbe. Just a question though: what is the knownBinsOnly option for?
PS: I initially forgot to run the test again (somehow) before pushing
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 default, binaries are expected to be shipped alongside the dependencies in node_modules/.bin - it's a setting that allows to be less strict and accept "any" binary name in CI environments—which are usually provisioned with unknown binaries—to prevent false positives for "unlisted binaries". However, the variable should def be improved.
| const app = config.app.split(" ")[0]; | ||
| const watch = config.watch?.include ?? []; | ||
| const context = Object.keys(config.context ?? {}).map(key => key.split("/")[0]) ?? []; | ||
| return compact([app, ...watch, ...context]).map(id => toDependency(id)); |
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 like that we're using compact here and already convert string to so-called "inputs" using toDependency. However, even though the context items are such dependencies indeed, returned items could also be entry points. You'll see what I mean when using getInputsFromScripts - its return value could contain various types of inputs.
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.
Are you meaning that I should be checking for the type of id in the map function? I saw typescript complaining about mismatched type and used something I saw somewhere else in the codebase:
compact([...app, ...watch, ...context]).map(id => (typeof id === 'string' ? toDependency(id) : id));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, something like that, e.g. the return value of getInputsFromScripts is inputs, not strings.
|
Thank you the pointers. I'll have a look asap. |
|
Where is the documentation for the plugin generated btw? I couldn't see where to add the meagre info that would allow someone to use/enable it. |
commit: |
The script to generate plugin docs is here: https://github.com/webpro-nl/knip/blob/main/packages/docs/scripts/generate-plugin-docs.ts. This is executed automatically for all plugins when docs are generated and deployed. Not sure when CloudFlare generates/deploys new docs for PRs. Maybe only for me as it did in e.g. #1029. But existing ones are all accessible from https://knip.dev/reference/plugins. |
|
@webpro Not sure what's the best approach to resolve the test failing. My understanding is that the test are failing because I have put a potential alternative path to the regular expected Should we consider this is not wanted, and revise it if people are coming back with the request for flexibility in defining the IaC path to the source code? Or do I need to create fixtures to get this test going? |
Not sure I understand the question. But if you would add The other unused devDependencies could also be removed from |
e764138 to
d4c7379
Compare
|
Just updated the branch and fix the tests: bun test v1.2.9 (9a329c04)
test/plugins/aws-cdk.test.ts:
{
files: Set {},
dependencies: {},
devDependencies: {
"package.json": {
"aws-cdk": [Object ...],
jest: [Object ...],
"ts-jest": [Object ...],
"ts-node": [Object ...],
},
},
optionalPeerDependencies: {},
unlisted: {},
binaries: {
"cdk.json": {
"ts-node": [Object ...],
},
},
unresolved: {},
exports: {},
nsExports: {},
types: {},
nsTypes: {},
enumMembers: {},
classMembers: {},
duplicates: {},
_files: {},
}
✓ Find dependencies with the aws-cdk plugin [119.78ms]
1 pass
0 fail
Ran 1 tests across 1 files. [587.00ms]How do I run all the tests to ensure we are good right now? |
I was referring to the possibility that the code base where I don't think it would make much of a difference right now. |
I often use this, from the Could you please remove the Do you know of a public repo we could use to test drive this plugin? To me it seems we have some good fixtures, but an actual repository would be great to verify we'd not be missing out on anything major. |
76027a6 to
6e17613
Compare
ea72d80 to
7787123
Compare
|
Apologies - I have not forgotten about this piece of work. I have just updates the branch and running the tests, there seems to be a good few errors now. I'll hunt down someone's public repo. All the example I have at hand are work related and have sensitive information about the company's setup. edit: that seems to be fitting the bill: https://github.com/aws-samples/extended-cdk-workshop-coffee-listing-app I have just cloned it, ran the setup for knip (as is) and ran > coffee-listing-app@0.1.0 knip
> knip
Unused files (10)
bin/coffee-listing-app.ts
frontend/src/App.jsx
frontend/src/index.jsx
frontend/src/Likes.jsx
frontend/src/PhotoList.jsx
frontend/src/UploadPhoto.jsx
lambdas/coffee-listing-api-public.ts
lib/coffee-listing-app-stack.ts
lib/rest-api-stack.ts
lib/website-hosting-stack.ts
Unused dependencies (3)
aws-cdk-lib package.json:28:6
constructs package.json:29:6
source-map-support package.json:30:6
Unused devDependencies (4)
@types/aws-lambda package.json:15:6
aws-sdk package.json:19:6
esbuild package.json:20:6
ts-node package.json:24:6Which seems to be a good candidate. Let me know how I could run my locally modified version to test the change. |
|
I have just re-ran the test as there was a change in the import: bun test test/plugins/aws-cdk.test.ts
bun test v1.2.15 (df017990)
test/plugins/aws-cdk.test.ts:
{
files: Set(1) {
"/Users/thomas.roche/Projects/github/webpro-nl/knip/packages/knip/fixtures/plugins/aws-cdk/bin/infra.ts",
},
dependencies: {},
devDependencies: {
"package.json": {
"aws-cdk": [Object ...],
jest: [Object ...],
"ts-jest": [Object ...],
"ts-node": [Object ...],
},
},
optionalPeerDependencies: {},
unlisted: {},
binaries: {
"cdk.json": {
"ts-node": [Object ...],
},
},
unresolved: {},
exports: {},
nsExports: {},
types: {},
nsTypes: {},
enumMembers: {},
classMembers: {},
duplicates: {},
_files: {
"fixtures/plugins/aws-cdk/bin/infra.ts": [
[Object ...]
],
},
}
✓ Find dependencies with the aws-cdk plugin [128.66ms]
1 pass
0 fail
Ran 1 tests across 1 files. [822.00ms] |
|
@thoroc just added a commit to assist a bit with the
Please see https://github.com/webpro-nl/knip/blob/main/.github/DEVELOPMENT.md |
5a2d249 to
6bd250a
Compare
|
Just checking in; What are we missing here? |
Afaic there has not been actual exercises on public repositories yet:
Were you able to run your local version of Knip with the dev guide? |
# Add support for aws-cdk
depsanddevDepsI need help on how to get
knipto find the dependencies in the most likely path (bin,cdk,lib) and to test for that. I also need to check for thecdk.jsonand the use ofts-nodein there (although it is already part of the dependencies check.an example of the
cdk.jsonfile is:{ "app": "ts-node --project=tsconfig.json --prefer-ts-exts cdk/bin/infra.ts", "watch": { "include": ["cdk/lib/**", "cdk/bin/**"] }, "context": { "aws-cdk-lib/core:enableDiffNoFail": true, "aws-cdk-lib/core:bootstrapQualifier": "custom-v2", "aws-cdk-lib/core:checkSecretUsage": true, "aws-cdk-lib/core:target-partitions": [ "aws", "aws-cn" ], "aws-cdk-lib/aws-apigateway:disableCloudWatchRole": true, "aws-cdk-lib/aws-apigateway:usagePlanKeyOrderInsensitiveId": true, "aws-cdk-lib/aws-cloudfront:defaultSecurityPolicyTLSv1.2_2021": false, "aws-cdk-lib/aws-lambda:recognizeLayerVersion": true } }Could you give me some pointers to resolve the above issues, and let me know if I am missing anything.
Ran the test for the new plugin using
bunv1.2.9:Let the commented out code and the
console.logfor now, but will be removing that when and if the MR is approved.NOTE
Just in case wondering this is only targetting v2 of the aws-cdk as v1 has been deprecated for a while now.