fix: extract Node.js/Yarn checks into shared script#9638
fix: extract Node.js/Yarn checks into shared script#9638Giggitycountless wants to merge 2 commits intowarpdotdev:masterfrom
Conversation
The Node.js and Yarn 4+ prerequisite checks were duplicated across 3 bootstrap scripts (linux/bootstrap, linux/install_build_deps, macos/bootstrap). Extract them into script/check_node_yarn_deps.sh and source it from each location. Also adds the prerequisite docs to WARP.md.
|
I ran into an unexpected error while working on this. I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment I reviewed this pull request and requested human review from: @warpdotdev/oss-maintainers. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR extracts Node.js/Yarn prerequisite validation into a shared script and sources it from Linux/macOS bootstrap paths.
Concerns
script/linux/install_build_depsis invoked by the CI prepare-environment action before that action installs Node and enables Corepack, so the new prerequisite check would fail Linux CI jobs before their existing setup-node step runs.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| fi | ||
|
|
||
| # Check Node.js and Yarn 4+ prerequisites. | ||
| source "$PWD/script/check_node_yarn_deps.sh" |
There was a problem hiding this comment.
captainsafia
added
the
external-contributor
Warp Dev Github Integration
CI calls install_build_deps before Node.js is installed, so the prerequisite check would fail. Keep the check only in the bootstrap scripts which are run by developers after their environment is set up.
|
/oz-review |
1 similar comment
|
/oz-review |
There was a problem hiding this comment.
Overview
This PR adds a shared Node.js/Yarn prerequisite script and wires it into the Linux and macOS bootstrap flows, with WARP.md documentation.
Concerns
- The inlined diff does not wire the shared check into
script/linux/install_build_deps, even though that script is a standalone Linux dependency setup path and the PR description says it should source the check. Direct callers still bypass the new validation.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| sudo apt update -y | ||
|
|
||
| # Check Node.js and Yarn 4+ prerequisites. | ||
| source "$PWD/script/check_node_yarn_deps.sh" |
There was a problem hiding this comment.
script/linux/install_build_deps is a standalone Linux setup path, but this diff only sources the shared check from bootstrap; source it from the build-deps script too so direct installs don't bypass the prerequisite validation.
There was a problem hiding this comment.
Overview
This PR extracts the Node.js and Yarn prerequisite checks into a shared bootstrap script, sources it from the Linux and macOS bootstrap flows, and documents the prerequisites in WARP.md.
Concerns
- No blocking correctness or security concerns found in the changed lines.
- The changed paths only match the catch-all stakeholder team in the provided STAKEHOLDERS rules, so no single eligible GitHub login could be selected for
recommended_reviewers.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
2 participants
The Node.js and Yarn 4+ prerequisite checks were duplicated across bootstrap scripts. This PR extracts them into a shared script that's sourced from each location.
Changes:
This is a cleaner version of #9571 that addresses the review feedback:
Fixes #9571