refactor(install-node.sh): simplify el/cl install scripts#120
refactor(install-node.sh): simplify el/cl install scripts#120coincashew merged 3 commits intomainfrom
Conversation
Walkthroughethpillar.sh now builds a lowercased filename deploy-.py from the selected client combo and invokes install-node.sh with that filename. install-node.sh was changed to accept, validate, and run a deploy-*.py install file. Legacy per-client installer scripts were removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant EthPillar as ethpillar.sh
participant Installer as install-node.sh
participant DeployPy as deploy-<client>.py
participant System
User->>EthPillar: select client combo (whiptail)
EthPillar->>EthPillar: build filename deploy-${CLIENTCOMBO,,}.py
EthPillar->>Installer: ./install-node.sh deploy-...py [skip_prompt]
Installer->>Installer: validate args and filename (deploy-*.py)
Installer->>DeployPy: python3 ~/git/ethpillar/${install_file}
DeployPy->>System: perform deployment steps
System-->>Installer: return status
Installer-->>User: "INSTALL COMPLETE! To start, type \"ethpillar\""
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ethpillar.sh (1)
1496-1505: Add existence check for deploy script before invoking installer
Guard against missing deploy-*.py files so users see a clear error instead of a cryptic failure:- _file="deploy-${_CLIENTCOMBO,,}.py" - runScript install-node.sh "${_file}" true + _file="deploy-${_CLIENTCOMBO,,}.py" + if [[ -f "$BASE_DIR/$_file" ]]; then + runScript install-node.sh "${_file}" true + else + whiptail --title "Missing deploy file" \ + --msgbox "Could not find $_file in $BASE_DIR.\nPlease update EthPillar and try again." 10 78 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
ethpillar.sh(1 hunks)install-lighthouse-reth.sh(0 hunks)install-node.sh(4 hunks)install-teku-besu.sh(0 hunks)
💤 Files with no reviewable changes (2)
- install-teku-besu.sh
- install-lighthouse-reth.sh
🧰 Additional context used
🧬 Code graph analysis (2)
install-node.sh (5)
install-nimbus-nethermind.sh (1)
linux_install_validator-install(123-133)deploy-nimbus-nethermind.py (1)
finish_install(809-898)install-teku-besu.sh (1)
linux_install_validator-install(123-133)deploy-lighthouse-reth.py (1)
finish_install(796-885)install-lighthouse-reth.sh (1)
linux_install_validator-install(123-133)
ethpillar.sh (3)
functions.sh (1)
runScript(162-179)install-nimbus-nethermind.sh (1)
linux_install_validator-install(123-133)install-teku-besu.sh (1)
linux_install_validator-install(123-133)
🔇 Additional comments (2)
install-node.sh (2)
165-168: Copy update LGTMClear, generic description. No action needed.
177-178: Completion banner LGTMConsistent with broader refactor.
| git clone https://github.com/coincashew/ethpillar.git ~/git/ethpillar 2> /dev/null || (cd ~/git/ethpillar ; git fetch origin main ; git checkout main ; git pull) | ||
| ohai "Installing validator-install" | ||
| $python ~/git/ethpillar/deploy-nimbus-nethermind.py | ||
| $python ~/git/ethpillar/${install_file} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Forward non-interactive flag to Python and quote path
Make skip behavior end-to-end by passing a CLI flag the deploy scripts understand, and quote expansions.
- $python ~/git/ethpillar/${install_file}
+ $python "$HOME/git/ethpillar/${install_file}" ${skip_prompt:+--skip-prompts}🤖 Prompt for AI Agents
In install-node.sh around line 133, the script invokes Python without forwarding
the non-interactive flag and without quoting the expanded path; update the call
to forward the deploy script's non-interactive option (e.g. --non-interactive)
and quote/avoid tilde by using $HOME so the path is expanded and safe, e.g. call
the Python variable with the flag followed by the quoted full path like: $python
--non-interactive "$HOME/git/ethpillar/${install_file}".
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install-node.sh (1)
136-143: Defer deploy-file existence check until after cloneAdd a post-clone guard so users get a clear error only if the file is truly missing.
Apply:
ohai "Cloning ethpillar into ~/git/ethpillar" mkdir -p ~/git/ethpillar git clone https://github.com/coincashew/ethpillar.git ~/git/ethpillar 2> /dev/null || (cd ~/git/ethpillar ; git fetch origin main ; git checkout main ; git pull) + # Ensure deploy file exists (post-clone) + if [[ ! -f "$HOME/git/ethpillar/$install_file" ]]; then + echo "ERROR: Missing file: $HOME/git/ethpillar/$install_file" + exit 1 + fi ohai "Installing validator-install"
♻️ Duplicate comments (2)
install-node.sh (2)
18-24: Use "$#" and validate arg count (0, 1, or 2) deterministicallyPrefer "$#" over "${#}" and reject >2 args to avoid silent ignores.
Apply:
-if [[ ${#} -eq 0 ]]; then +if [[ $# -eq 0 ]]; then echo "ERROR: Missing deploy file. Example ./install-node.sh deploy-nimbus-nethermind.py" exit 1 -elif [[ ${#} -eq 2 ]]; then +elif [[ $# -eq 2 ]]; then skip_prompt="$2" +elif [[ $# -gt 2 ]]; then + echo "ERROR: Too many arguments. Usage: ./install-node.sh <deploy-*.py> [--skip-prompts]" + exit 1 fi install_file="$1"
141-141: Run the deploy with the venv’s Python, quote path, and propagate non-interactive flagCurrently this uses system python and doesn’t forward skip; packages were installed into ~/.local venv, so imports may fail.
Apply:
- $python ~/git/ethpillar/${install_file} + "$HOME/.local/bin/python" "$HOME/git/ethpillar/${install_file}" ${skip_prompt:+--skip-prompts} + rc=$? + exit_on_error $rc "deploy: ${install_file}"
🧹 Nitpick comments (1)
install-node.sh (1)
26-29: Harden filename validationGood safeguard. Consider a stricter whitelist and forbid leading dashes.
Apply:
-# Accept only deploy-*.py and disallow slashes -if [[ "$install_file" != deploy-*.py || "$install_file" == */* ]]; then +# Accept only safe filenames: deploy-<alnum,._->.py; no slashes; no leading '-' +if [[ "$install_file" == */* || "$install_file" == -* || ! "$install_file" =~ ^deploy-[A-Za-z0-9._-]+\.py$ ]]; then echo "ERROR: Invalid deploy file: $install_file" exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
install-node.sh(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
install-node.sh (1)
deploy-nimbus-nethermind.py (3)
finish_install(809-898)install_nimbus_validator(757-807)install_nimbus(681-736)
🔇 Additional comments (2)
install-node.sh (2)
172-176: Message update LGTMClearer generic description.
185-185: Completion banner LGTMConcise next-step instruction.
| # Ensure file exists before continuing | ||
| if [[ ! -f "$HOME/git/ethpillar/$install_file" ]]; then | ||
| echo "ERROR: Missing file: $HOME/git/ethpillar/$install_file" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Existence check runs before repo clone — causes first-run failure
You verify $HOME/git/ethpillar/$install_file before cloning the repo (done later), so first-time installs will exit early.
Apply (remove pre-check here; re-check after clone):
-# Ensure file exists before continuing
-if [[ ! -f "$HOME/git/ethpillar/$install_file" ]]; then
- echo "ERROR: Missing file: $HOME/git/ethpillar/$install_file"
- exit 1
-fiThen, add the check right after cloning (see suggested patch for lines 136-143 in a separate comment).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Ensure file exists before continuing | |
| if [[ ! -f "$HOME/git/ethpillar/$install_file" ]]; then | |
| echo "ERROR: Missing file: $HOME/git/ethpillar/$install_file" | |
| exit 1 | |
| fi | |
| # (Removed pre-clone existence check to avoid first-run failure) |
🤖 Prompt for AI Agents
In install-node.sh around lines 30 to 34, the script checks for
$HOME/git/ethpillar/$install_file before the repository is cloned which causes
first-run installs to fail; remove this pre-check block here and instead add the
existence check immediately after the repo clone (around lines 136-143) so the
script verifies the file only after cloning, exiting with an error if the file
is still missing.
Summary by CodeRabbit
New Features
Refactor
Chores / Bug Fixes