-
Notifications
You must be signed in to change notification settings - Fork 4k
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
chore: add "gen" target as a pre build script #10648
Changes from 9 commits
89b3ef7
54f5362
9712bfa
16bbec7
f3d75ee
5799458
056eee7
f259487
51d3a24
02a6a66
3e0d2d6
794b996
ff2f119
8af1c35
dfcb714
c8a944b
0f737ce
8e1cfc3
3f5b4bf
cf245a4
5a717f0
8980c3e
e6c684d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,11 +50,15 @@ | |
"cfn2ts": "cfn2ts", | ||
"build+test+package": "npm run build+test && npm run package", | ||
"build+test": "npm run build && npm test", | ||
"compat": "cdk-compat" | ||
"compat": "cdk-compat", | ||
"gen": "cfn2ts" | ||
}, | ||
"cdk-build": { | ||
"cloudformation": "Alexa::ASK", | ||
"jest": true, | ||
"pre": [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels unnecessary. We can just code knowledge of the (Yes, that means moving everything that's currently in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call. I removed I moved all code generation logic to |
||
"npm run gen" | ||
], | ||
"env": { | ||
"AWSLINT_BASE_CONSTRUCT": true | ||
} | ||
|
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.
Why don't we put the arguments to cfn2ts here as well? Let's make this
etc.
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.
We don't need it,
cfn2ts
already gets this from thepacakge.json
file:aws-cdk/tools/cfn2ts/bin/cfn2ts.ts
Lines 43 to 46 in bc096f0
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 know, but it gets it from the
cdk-build
section. Don't you agree that's kinda confusing because we're not usingcdk-build
to invokecfn2ts
anymore?In any case, fine.
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 agree it's confusing, as evident by having to read through
cfn2ts
code to understand why calling it without the scope name even works. I think we can add it to thegen
directive for clarity, not sure about removing the logic fromcfn2ts
, it is a private package so technically we wont be breaking anyone.