fix(integ-runner): Fix call to spawnSync for hooks commands #22429

arewa · 2022-10-08T17:04:45Z

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:

Have you followed the guidelines in our Contributing guide?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-10-08T17:04:53Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

TheRealAmazonKendra · 2022-10-14T18:29:08Z

packages/@aws-cdk/integ-tests/test/integ.app.ts

@@ -0,0 +1,7 @@
+import { App, Stack } from '@aws-cdk/core';


This isn't testing anything. Why is this here?

@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.

TheRealAmazonKendra · 2022-10-14T18:29:50Z

packages/@aws-cdk/integ-tests/package.json

@@ -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"


This isn't needed in this package.

TheRealAmazonKendra · 2022-10-14T18:34:21Z

packages/@aws-cdk/integ-tests/package.json

@@ -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",


Pull request has been modified.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

corymhall · 2022-10-18T17:23:51Z

packages/@aws-cdk/integ-runner/lib/utils.ts

+  let chunks = command.split(/\s+/);
+  const cmd = chunks.shift();
+  if (cmd !== undefined) {
+    return [cmd, chunks.join(' ')];


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?

@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.

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.

Yes, make sense, I updated the solution. Thank you again for the review.

corymhall

Looks great! just one minor nit and then we are good to go!

corymhall · 2022-10-19T18:13:14Z

packages/@aws-cdk/integ-runner/lib/utils.ts

+ */
+export function chunks(command: string): string[] {
+  const result = command.match(/(?:[^\s"]+|"[^"]*")+/g);
+  if (result !== null) {


nit

return result ?? [];

Exactly, updated ))

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

aws-cdk-automation · 2022-10-19T20:02:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5c93451
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-10-19T20:02:37Z

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*

fix(integ-runner): Fix call to spawnSync for hooks commands

a91c3c3

github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Oct 8, 2022

aws-cdk-automation requested a review from a team October 8, 2022 17:04

github-actions bot added the p2 label Oct 8, 2022

aws-cdk-automation previously requested changes Oct 8, 2022

View reviewed changes

arewa marked this pull request as draft October 8, 2022 17:07

arewa marked this pull request as ready for review October 8, 2022 17:11

arewa added 5 commits October 8, 2022 21:57

Additional tests

1b798f3

Merge branch 'main' into issue-22344

268a3f6

Merge branch 'main' into issue-22344

c19f2ff

Merge branch 'main' into issue-22344

1661e8f

Merge branch 'main' into issue-22344

a0d433f

arewa marked this pull request as draft October 11, 2022 12:44

Add sample integration test

f71d813

arewa marked this pull request as ready for review October 11, 2022 14:14

Merge branch 'main' into issue-22344

8c44451

arewa requested a review from aws-cdk-automation October 11, 2022 14:15

arewa added 4 commits October 12, 2022 10:41

Merge branch 'main' into issue-22344

049852a

Merge branch 'main' into issue-22344

2021e09

Merge branch 'main' into issue-22344

85c88b3

Merge branch 'main' into issue-22344

4f5e259

TheRealAmazonKendra previously requested changes Oct 14, 2022

View reviewed changes

Remove excess integ test

b15c9a8

Remove excess snapshots

24f7ba8

arewa requested review from TheRealAmazonKendra and removed request for aws-cdk-automation October 14, 2022 18:55

aws-cdk-automation previously requested changes Oct 14, 2022

View reviewed changes

corymhall suggested changes Oct 18, 2022

View reviewed changes

Merge branch 'main' into issue-22344

b015106

arewa requested review from corymhall and TheRealAmazonKendra and removed request for TheRealAmazonKendra and corymhall October 19, 2022 11:58

arewa added 4 commits October 19, 2022 19:55

Merge branch 'main' into issue-22344

3fcd260

Merge branch 'main' into issue-22344

952b8a9

Refator the solution

937e43d

Tiny update

c5ed430

corymhall suggested changes Oct 19, 2022

View reviewed changes

arewa added 3 commits October 19, 2022 21:44

Tiny update

a899f7f

Merge branch 'main' into issue-22344

91f6884

Merge branch 'main' into issue-22344

5c93451

corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Oct 19, 2022

corymhall approved these changes Oct 19, 2022

View reviewed changes

mergify bot merged commit 9139ca9 into aws:main Oct 19, 2022

comcalvi mentioned this pull request Aug 19, 2024

(integ-runner): Hooks do not follow spawnSync argument requirements #22344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(integ-runner): Fix call to spawnSync for hooks commands #22429

fix(integ-runner): Fix call to spawnSync for hooks commands #22429

arewa commented Oct 8, 2022 •

edited

Loading

gitpod-io bot commented Oct 8, 2022

aws-cdk-automation left a comment

TheRealAmazonKendra Oct 14, 2022

arewa Oct 14, 2022

TheRealAmazonKendra Oct 14, 2022

TheRealAmazonKendra Oct 14, 2022

aws-cdk-automation left a comment

corymhall Oct 18, 2022

arewa Oct 19, 2022

corymhall Oct 19, 2022

arewa Oct 19, 2022

corymhall left a comment

corymhall Oct 19, 2022

arewa Oct 19, 2022

aws-cdk-automation commented Oct 19, 2022

mergify bot commented Oct 19, 2022

fix(integ-runner): Fix call to spawnSync for hooks commands #22429

fix(integ-runner): Fix call to spawnSync for hooks commands #22429

Conversation

arewa commented Oct 8, 2022 • edited Loading

All Submissions:

gitpod-io bot commented Oct 8, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 19, 2022

AWS CodeBuild CI Report

mergify bot commented Oct 19, 2022

arewa commented Oct 8, 2022 •

edited

Loading