-
Notifications
You must be signed in to change notification settings - Fork 246
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
feat: CDK Build Integration Test #1219
feat: CDK Build Integration Test #1219
Conversation
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
a99ddf7
to
ca6bf1b
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Noice! Looking solid at this point... some super minor comments, but nothing blocking.
Waiting for you to figure out why the PR build does not pass & fix that :)
ca6bf1b
to
342020b
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
342020b
to
ae2cbb8
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
ae2cbb8
to
1e76dd6
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
1e76dd6
to
6560d16
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Minor comments only!
await processes.spawn('npx', [ | ||
'lerna', | ||
'run', | ||
'--stream', | ||
'--scope', | ||
'@aws-cdk/*', | ||
'--scope', | ||
'aws-cdk', | ||
'build', | ||
'--', | ||
'--jsii', | ||
path.join(JSII_DIR, 'bin', 'jsii'), | ||
], { cwd: srcDir }); |
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.
Instead of using the --jsii path/to/jsii
argument, why not use the CDK_BUILD_JSII
environment variable? Is there a difference?
@@ -0,0 +1,8 @@ | |||
{ | |||
"compilerOptions": { | |||
"composite": false, |
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? I reckon this also disables incremental
...
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.
tsc throws tsconfig.json(3,5): error TS6304: Composite projects may not disable declaration emit.
?
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.
Oh yeah, right. Composite requires declarations... but why bother?
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.
yeah idk, I wasn't quite sure the ideal solve here. Outputting declarations is fine? Just doesn't feel like we should need to do.
6560d16
to
241c5a7
Compare
Wait, why not clone the |
Adds @jsii/integ-test private module for defining new integration tests. Adds a new integration test that downloads the latest CDK release source code and builds it with the local version of jsii and jsii-pacmak. This unit test requires a github access token defined in the environment to get the latest release version and download the asset. Fixes: aws#1209
Co-Authored-By: Romain Marcadier-Muller <[email protected]>
Use the tar node module instead of shelling out to tar. This allows us to stream the download contents directly to unpacking and removes the need to manually parse the beginning of the commit id hash to traverse into the build directory.
We use the token to hit the GH API and get the tag of the latest release. Then using that tag we download the source from the archive. Neither of these actually requires a token but it gets rate limited pretty quickly without one. Doing a |
241c5a7
to
e90246f
Compare
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Adds @jsii/integ-test private module for defining new integration tests.
Adds a new integration test that downloads the latest CDK release source
code and builds it with the local version of jsii and jsii-pacmak.
This unit test requires a github access token defined in the environment
to get the latest release version and download the asset.
Fixes: #1209
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.