fix(lambda-nodejs): run beforeInstall hook after workspace files are written - #37899
fix(lambda-nodejs): run beforeInstall hook after workspace files are written#37899Zelys-DFKH wants to merge 6 commits into
Conversation
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Exemption Request This fix reorders internal bundling steps: it splits |
…written Splits the node-modules file-ops step into two phases — prepareSteps (writes pnpm-workspace.yaml, package.json, and the lockfile) and installSteps (runs the package manager and cleanup) — so that the beforeInstall command hook fires between them. Previously, beforeInstall ran before depsCommand, which meant CDK's empty pnpm-workspace.yaml write always silently overrode any allowBuilds entries a user appended there. This made the hook useless for pnpm v11 workspaces that need allowBuilds to permit native build scripts (e.g. cpu-features via ssh2-sftp-client). Fixes aws#37898. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rdering Adds two layers of test coverage for the ordering fix from the previous commit: 1. E2e behavioral test in bundling-e2e.test.ts — asserts that allowBuilds written to pnpm-workspace.yaml by beforeInstall survives CDK's own workspace file setup. Runs under both local (skipped if pnpm absent) and Docker bundling. Closes the test gap that allowed the bug to exist. 2. CDK integration test (integ.dependencies-pnpm-before-install.ts) — deploys a Lambda that uses delay@5.0.0 with a beforeInstall hook writing to pnpm-workspace.yaml, exercising the full bundling pipeline. Snapshot hashes are placeholder (all-zeros); run `yarn integ` in framework-integ to regenerate from the Docker bundled output. Fixes aws#37898 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The integ test required Docker + AWS credentials to generate real asset hashes, blocking CI. The unit test in bundling.test.ts directly asserts the ordering fix (pnpm-workspace.yaml write → beforeInstall → pnpm install) and the e2e test in bundling-package-managers-e2e.test.ts validates the behavioral fix end-to-end without a live deploy.
…ing test Docker bundling always builds commands with osPlatform 'linux', so OsCommand.write escapes the output path via posixShellEscape and emits single quotes. The assertion matched the win32 double-quoted form, which that code path never produces, so it failed regardless of hook ordering. The behaviour under test is unchanged: the workspace file is written, then the beforeInstall hook runs, then pnpm install.
|
Thanks for writing this up, @marco2216. The Your case is the same write this PR reorders, just surfacing as slowness rather than a failure. The blank It also sharpens the pending exemption question, since a template snapshot can't see your symptom either. Synthesis output is byte-identical either way. The only thing that changes is when the hook runs relative to the file write. Three reports now (@cmodijk, @pivstone, @marco2216), three symptoms, one ordering bug. Branch is updated against main. |
Fixes #37898.
Credit to @cmodijk: the issue report traces the execution order exactly, identifies the root cause, and spells out what the fix should look like. This PR implements what you described.
The problem
On pnpm v11,
strictDepBuildsis the effective default. AnyNodejsFunctionthat bundlesnodeModuleswith a native-build dependency (e.g.ssh2-sftp-client→cpu-features) hits this at deploy time:The
beforeInstallhook was designed to handle this: appendallowBuildsentries topnpm-workspace.yamlbefore the install runs. It silently doesn't work, which is worse than not existing.Why
CDK writes an empty
pnpm-workspace.yamlto prevent pnpm from walking up to a monorepo root (introduced in #21910, still the right call). The bug was ordering. Before this fix:Whatever the user wrote in
beforeInstallgot overwritten a step later. The hook's name is a lie.Fix
Splits node-modules file-ops into two phases via a new
NodeModuleFileOpsinterface:prepareSteps: writespnpm-workspace.yaml,package.json, and the lockfileinstallSteps: runs the package manager and post-install cleanupcreateBundlingStepsputsbeforeInstallbetween them, for both Docker and local bundling.No behavior change for callers where
beforeInstallreturns[](the default).The refactor makes
createBundlingStepsslightly more involved.dockerFileOpsandlocalFileOpsare both private, so there's no API surface change.Validation
Unit test (
bundling.test.ts): ThebeforeInstall hook fires after pnpm workspace files are written (Docker)test asserts the Docker shell command orderspnpm-workspace.yamlwrite →beforeInstall→pnpm install. This is the test that would have caught the bug.E2E behavioral test (
bundling-package-managers-e2e.test.ts): Thepnpm-specific / beforeInstall writes to pnpm-workspace.yaml survive CDK workspace setuptest runs CDK synthesis with a minimal pnpm project (local and Docker where available), writesallowBuilds:topnpm-workspace.yamlinbeforeInstall, reads the output file, and checks the content survived. Skipped when pnpm is unavailable locally and Docker bundling is off.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license