fix(scripts): findNodeProcess compatible busybox ps command#5874
Conversation
Fixed an error when executing the `npm stop` command within the Alpine Node.js container in Kubernetes. Signed-off-by: nAnderYang <nander@qq.com>
📝 WalkthroughWalkthroughModified non-Windows process enumeration in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the process discovery logic in helper.ts to support BusyBox environments, which are common in Alpine Linux. The shell command now detects BusyBox and uses compatible flags for the ps command. Feedback was provided to simplify the command by removing a redundant existence check, as the fallback mechanism already handles missing binaries.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/scripts/src/helper.ts (1)
14-19: Optional: extract the shell command and/or cache BusyBox detection.The inline command string is hard to read and re-runs
ps --help | grep BusyBoxon every call (andfindNodeProcessis invoked multiple times perstoprun, seecommands/stop.ts). Two small cleanups would help readability and avoid repeating the detection probe:♻️ Suggested refactor
-export async function findNodeProcess(filterFn?: FilterFunction): Promise<NodeProcess[]> { - const command = isWindows - ? 'wmic Path win32_process Where "Name = \'node.exe\'" Get CommandLine,ProcessId' - : // command, cmd are alias of args, not POSIX standard, so we use args - 'command -v ps >/dev/null 2>&1 && ps --help 2>&1 | grep -q BusyBox && ps -o "pid,args" || ps -wweo "pid,args"'; +const WIN_PS_COMMAND = + 'wmic Path win32_process Where "Name = \'node.exe\'" Get CommandLine,ProcessId'; +// `command`/`cmd` are aliases of `args` and not POSIX-standard, so we use `args`. +// BusyBox `ps` (e.g. Alpine) does not accept `-wweo`; detect it and fall back to `-o`. +const NIX_PS_COMMAND = + 'if ps --help 2>&1 | grep -q BusyBox; then ps -o "pid,args"; else ps -wweo "pid,args"; fi'; + +export async function findNodeProcess(filterFn?: FilterFunction): Promise<NodeProcess[]> { + const command = isWindows ? WIN_PS_COMMAND : NIX_PS_COMMAND;Using
if … then … else … fialso avoids theA && B || Cprecedence pitfall: with the current expression, if BusyBox is detected butps -o "pid,args"exits non-zero, execution falls through tops -wweo "pid,args"(which will also fail on BusyBox).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/src/helper.ts` around lines 14 - 19, findNodeProcess currently builds a dense inline shell command and re-runs the BusyBox probe on every call; extract the platform-specific command construction into a small helper (e.g. getPsCommand or buildProcessCommand) and cache BusyBox detection in a module-level boolean (e.g. isBusyBox) to avoid repeated probes; change the BusyBox probe to use an explicit if…then…else shell snippet (not A && B || C) when composing the non-Windows command, and update findNodeProcess to call runScript with the returned command. Reference symbols: findNodeProcess, runScript, isWindows, and the new helper (getPsCommand/isBusyBox) so the command string is clearer and detection runs once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/scripts/src/helper.ts`:
- Around line 14-19: findNodeProcess currently builds a dense inline shell
command and re-runs the BusyBox probe on every call; extract the
platform-specific command construction into a small helper (e.g. getPsCommand or
buildProcessCommand) and cache BusyBox detection in a module-level boolean (e.g.
isBusyBox) to avoid repeated probes; change the BusyBox probe to use an explicit
if…then…else shell snippet (not A && B || C) when composing the non-Windows
command, and update findNodeProcess to call runScript with the returned command.
Reference symbols: findNodeProcess, runScript, isWindows, and the new helper
(getPsCommand/isBusyBox) so the command string is clearer and detection runs
once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1ea6939-a9e3-4e1b-a441-c88517062ab5
📒 Files selected for processing (1)
tools/scripts/src/helper.ts
remove redundant check commands as recommended. Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: nAnderYang <nander@qq.com>
|
@fengmk2 please review |
fengmk2
changed the title
|
@codex review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5874 +/- ##
=======================================
Coverage 85.49% 85.49%
=======================================
Files 660 660
Lines 18828 18828
Branches 3646 3646
=======================================
Hits 16097 16097
Misses 2360 2360
Partials 371 371 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Fixed an error when executing the
npm stopcommand within the Alpine Node.js container in Kubernetes.Error
The
busybox psno other parameterSummary by CodeRabbit