findNodeProcess compatible bosybox ps command#65
findNodeProcess compatible bosybox ps command#65nAnderYang wants to merge 1 commit intoeggjs:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the process discovery logic in src/helper.ts to handle BusyBox environments by detecting the ps implementation and adjusting flags accordingly. Feedback suggests that the BusyBox-specific command should include -w and -A flags to ensure all processes are listed and command arguments are not truncated.
| 'wmic Path win32_process Where "Name = \'node.exe\'" Get CommandLine,ProcessId' : | ||
| // command, cmd are alias of args, not POSIX standard, so we use args | ||
| 'ps -wweo "pid,args"'; | ||
| 'command -v ps >/dev/null 2>&1 && ps --help 2>&1 | grep -q BusyBox && ps -o "pid,args" || ps -wweo "pid,args"'; |
There was a problem hiding this comment.
The ps -o "pid,args" command for BusyBox is likely insufficient for two reasons:
- Missing all-process flag: By default, BusyBox
psoften only shows processes associated with the current terminal and user. To find background server processes (master and workers), you should use-Aor-eto ensure all processes are listed. - Truncation: BusyBox
psoutput is typically truncated to the terminal width. Adding the-wflag is necessary to ensure the full command line is captured, which is critical for thefilterFnand the logic instop.tsthat relies on matching specific strings (likestart-cluster) in the process arguments.
I suggest using ps -w -A -o pid,args for the BusyBox case.
| 'command -v ps >/dev/null 2>&1 && ps --help 2>&1 | grep -q BusyBox && ps -o "pid,args" || ps -wweo "pid,args"'; | |
| 'command -v ps >/dev/null 2>&1 && ps --help 2>&1 | grep -q BusyBox && ps -w -A -o pid,args || ps -wweo "pid,args"'; |
There was a problem hiding this comment.
alpine(buxybox) the ps command does not include the -w and -A parameter.
The issue of
egg-scripts stopfailing to run on Alpine (Buxybox) systems.Summary by CodeRabbit