fix(integ-runner): Fix call to spawnSync for hooks commands #22429
Conversation
|
|
github-actions
bot
added
the
repeat-contributor
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
| @@ -0,0 +1,7 @@ | |||
| import { App, Stack } from '@aws-cdk/core'; | |||
There was a problem hiding this comment.
This isn't testing anything. Why is this here?
There was a problem hiding this comment.
@TheRealAmazonKendra, I can actually remove this I added it to avoid the linter issue saying PR must have at least one integration test update. But integ-runner doesn't have its own integration tests that's why I added this small test. And with this test integ-tests/package.json will also be reverted.
| @@ -52,7 +52,8 @@ | |||
| "compat": "cdk-compat", | |||
| "rosetta:extract": "yarn --silent jsii-rosetta extract", | |||
| "build+extract": "yarn build && yarn rosetta:extract", | |||
| "build+test+extract": "yarn build+test && yarn rosetta:extract" | |||
| "build+test+extract": "yarn build+test && yarn rosetta:extract", | |||
| "integ": "integ-runner" | |||
There was a problem hiding this comment.
This isn't needed in this package.
| @@ -65,6 +66,7 @@ | |||
| "@aws-cdk/cdk-build-tools": "0.0.0", | |||
| "@aws-cdk/cx-api": "0.0.0", | |||
| "@aws-cdk/pkglint": "0.0.0", | |||
| "@aws-cdk/integ-runner": "0.0.0", | |||
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
arewa
requested review from
TheRealAmazonKendra
and removed request for
aws-cdk-automation
| let chunks = command.split(/\s+/); | ||
| const cmd = chunks.shift(); | ||
| if (cmd !== undefined) { | ||
| return [cmd, chunks.join(' ')]; |
There was a problem hiding this comment.
Should this be return chunks instead?
For example if I had
hooks: {
preDeploy: ['npx cdk deploy'],
}
I would want it to return ['npx', 'cdk', 'deploy'] right?
There was a problem hiding this comment.
@corymhall, thank you for the comment, for the command you mentioned it could be like that. But for example for the command echo "preDeploy hook" if we just split it by space it will result in ['echo', '"preDeploy', 'hook"'] which seems to be not the correct input for exec.
There was a problem hiding this comment.
You could do something like this instead in order to keep quoted sections together.
let chunks = command.match(/(?:[^\s"]+|"[^"]*")+/g)
// ['echo', '"preDeploy hook"']I think if you use arguments with spawnSync each argument to the command needs to be its own element in the list.
There was a problem hiding this comment.
Yes, make sense, I updated the solution. Thank you again for the review.
arewa
requested review from
corymhall and
TheRealAmazonKendra
and removed request for
TheRealAmazonKendra and
corymhall
| */ | ||
| export function chunks(command: string): string[] { | ||
| const result = command.match(/(?:[^\s"]+|"[^"]*")+/g); | ||
| if (result !== null) { |
There was a problem hiding this comment.
nit
return result ?? [];
corymhall
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
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). |
Fixes the issue #22344 I reworked the approach of calling `exec` by splitting each command in hook to the command itself and it's arguments. All hooks were affected: preDeploy, postDeploy, preDestroy, postDestroy. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Fixes the issue aws#22344 I reworked the approach of calling `exec` by splitting each command in hook to the command itself and it's arguments. All hooks were affected: preDeploy, postDeploy, preDestroy, postDestroy. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Fixes the issue #22344
I reworked the approach of calling
execby splitting each command in hook to the command itself and it's arguments. All hooks were affected: preDeploy, postDeploy, preDestroy, postDestroy.All Submissions:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license