fix: run iii console installer with bash#801
Conversation
|
@mturac is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR updates the iii-console installer to require and use ChangesShell switch to bash
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (2)
src/cli.ts (1)
662-662: 💤 Low valueLogin shell flag (
-lc) for install script.The
-lcflag runs bash as a login shell, which loads user profiles and environment. This differs from the engine installer at line 737 which usessh -c. The choice is likely intentional to ensure the console install script runs with a fully-configured bash environment, though-cmight suffice if the script doesn't depend on login-shell setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` at line 662, The call to runCommand using bashBin with flags ["-lc", III_CONSOLE_INSTALL_CMD] is inconsistent with the engine installer (which uses "sh -c") and may unnecessarily invoke a login shell; change the flags to ["-c", III_CONSOLE_INSTALL_CMD] in the runCommand invocation (referencing bashBin and III_CONSOLE_INSTALL_CMD) unless the console installer actually requires login-shell profile loading—if it does, add a brief comment above the runCommand explaining why "-lc" is required.test/iii-console-install.test.ts (1)
4-15: 💤 Low valueRegression test uses source string matching.
This test validates the bash switch by reading
src/cli.tsand checking for literal strings. While brittle (formatting changes or comments mentioning "install.sh | sh" would break it), this approach is acceptable for a focused regression test guarding against issue#712. The negative assertion on line 13 ensures the problematicshvariant doesn't reappear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/iii-console-install.test.ts` around lines 4 - 15, The test is brittle because it matches exact source strings in src/cli.ts; change the assertions to be resilient to minor formatting changes by using regex matches instead of exact string comparisons: replace expect(cli).toContain("curl -fsSL https://install.iii.dev/iii/main/install.sh | bash") with a toMatch(/curl -fsSL\s+https:\/\/install\.iii\.dev\/iii\/main\/install\.sh\s*\|\s*bash/), replace expect(cli).toContain('const bashBin = whichBinary("bash");') and the runCommand check with regexes like toMatch(/const\s+bashBin\s*=.*whichBinary\(["']bash["']\)/) and toMatch(/runCommand\(\s*bashBin\s*,\s*\[.*"-lc".*\]/), and keep the negative assertion using a regex for /install\.sh\s*\|\s*sh/ to ensure the "sh" variant is not present while allowing harmless formatting differences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cli.ts`:
- Line 662: The call to runCommand using bashBin with flags ["-lc",
III_CONSOLE_INSTALL_CMD] is inconsistent with the engine installer (which uses
"sh -c") and may unnecessarily invoke a login shell; change the flags to ["-c",
III_CONSOLE_INSTALL_CMD] in the runCommand invocation (referencing bashBin and
III_CONSOLE_INSTALL_CMD) unless the console installer actually requires
login-shell profile loading—if it does, add a brief comment above the runCommand
explaining why "-lc" is required.
In `@test/iii-console-install.test.ts`:
- Around line 4-15: The test is brittle because it matches exact source strings
in src/cli.ts; change the assertions to be resilient to minor formatting changes
by using regex matches instead of exact string comparisons: replace
expect(cli).toContain("curl -fsSL https://install.iii.dev/iii/main/install.sh |
bash") with a toMatch(/curl
-fsSL\s+https:\/\/install\.iii\.dev\/iii\/main\/install\.sh\s*\|\s*bash/),
replace expect(cli).toContain('const bashBin = whichBinary("bash");') and the
runCommand check with regexes like
toMatch(/const\s+bashBin\s*=.*whichBinary\(["']bash["']\)/) and
toMatch(/runCommand\(\s*bashBin\s*,\s*\[.*"-lc".*\]/), and keep the negative
assertion using a regex for /install\.sh\s*\|\s*sh/ to ensure the "sh" variant
is not present while allowing harmless formatting differences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90eb052c-c16f-47fd-ad59-8130ea2b6572
📒 Files selected for processing (2)
src/cli.tstest/iii-console-install.test.ts
Summary
install.sh | shTests
Closes #712
Summary by CodeRabbit
Bug Fixes
bashinstead ofshfor improved compatibility and feature support.bashandcurlavailability before attempting installation and warns if either tool is missing.Tests