-
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(CLI-integ-tests): cli integ tests cannot use local CDK framework #31131
Conversation
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
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 essentially skips install of the CDK and uses the local copy, correct? Just getting clarification, then good to approve.
captureStderr: false, | ||
cwd: root, | ||
show: 'error', | ||
})); | ||
})).data; | ||
const output: YarnWorkspacesOutput = JSON.parse(outputDataString); |
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 do we need to parse the shell output again?
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 first need to parse the output of yarn workspaces
, which includes a data
object. That data
object is an escaped JSON string, which needs to be parsed again.
@@ -96,7 +97,7 @@ async function findYarnPackages(root: string): Promise<Record<string, string>> { | |||
* Find the root directory of the repo from the current directory | |||
*/ | |||
export async function autoFindRoot() { | |||
const found = await findUp('release.json'); | |||
const found = findUp('release.json'); |
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.
Odd that this was ever awaited. Looking in the findUp
function, I am not seeing async calls, but is there any chance this await is necessary?
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.
TS compiler said it's not needed, so I removed it. It doesn't return a promise, so there's no need to await anything here.
Yep, it skips installing the CDK and uses the local version. |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Reason for this change
The cli integration tests cannot use your local version of
aws-cdk-lib
. This can be verified by making yourStack
construct throw an error upon creation, and watching no CLI integration tests fail, even with-a
.Description of changes
Fixed the CLI integration test framework to correctly link the local packages, like it used to.
Description of how you validated changes
Manual testing. This isn't something we can add automated tests for.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license