Added preinstall script #955
Conversation
WalkthroughA new shell script named Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
preinstall.sh (2)
1-2: Add error handling and cleanup.The script would benefit from proper error handling and cleanup of temporary files.
Add these at the beginning of the script:
#!/bin/bash +set -e # Exit on error + +# Cleanup function +cleanup() { + # Add cleanup tasks here + rm -f "$BREW_SCRIPT" 2>/dev/null || true +} + +# Set up trap +trap cleanup EXIT
1-53: Improve documentation and user feedback.The script would benefit from better documentation and more informative messages.
Add a header comment block and improve messaging:
#!/bin/bash + +# preinstall.sh +# +# This script ensures that required system packages (jq and bc) are installed +# before running the scriptMaster script. It supports Linux (with apt) and +# macOS (with Homebrew) systems. +# +# Required packages: +# - jq: Command-line JSON processor +# - bc: Basic calculator -echo "Setting up the development environment..." +echo "🔧 Setting up the development environment..."🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 42-42: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)preinstall.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
preinstall.sh
[warning] 42-42: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
package.json (1)
56-56: Consider adding a postinstall verification step.While the preinstall script is correctly added, consider adding a verification step to ensure the required packages are available after installation.
Add a verification script:
"preinstall": "bash preinstall.sh", + "postinstall": "command -v jq >/dev/null && command -v bc >/dev/null || (echo 'Required packages not found after installation' && exit 1)",Also, verify that this change doesn't conflict with the existing postinstall script:
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 lines) |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
preinstall.sh (3)
6-15:⚠️ Potential issueImprove security and robustness of Linux package installation.
The current implementation has several security and robustness concerns that need to be addressed.
Apply this diff to improve security and robustness:
+readonly VALID_PACKAGES=("jq" "bc") + install_package_linux() { PACKAGE=$1 + + # Validate package is in allowed list + if [[ ! " ${VALID_PACKAGES[@]} " =~ " ${PACKAGE} " ]]; then + echo "Invalid package name: $PACKAGE" + exit "$E_PACKAGE" + fi + + # Check for apt-get availability + if ! command -v apt-get &> /dev/null; then + echo "apt-get not found. This script requires apt-based Linux distribution." + exit "$E_GENERAL" + fi + if ! command -v "$PACKAGE" &> /dev/null; then echo "Installing $PACKAGE on Linux..." - sudo apt-get update -y - sudo apt-get install -y $PACKAGE + if ! sudo apt-get update -y; then + echo "Failed to update package list" + exit "$E_PACKAGE" + fi + if ! sudo apt-get install -y "$PACKAGE"; then + echo "Failed to install $PACKAGE" + exit "$E_PACKAGE" + fi else echo "$PACKAGE is already installed." fi }
30-34: 🛠️ Refactor suggestionAdd sudo availability check for Linux.
The script should verify sudo availability before proceeding with Linux installation.
if [[ "$OS" == "Linux" ]]; then echo "Detected Linux. Using apt package manager." + # Check for sudo availability + if ! command -v sudo &> /dev/null; then + echo "sudo is required but not available" + exit "$E_GENERAL" + fi install_package_linux jq install_package_linux bc
37-56:⚠️ Potential issueImprove Homebrew installation security.
The Homebrew installation process needs additional security measures.
- Verify the downloaded script's checksum
- Use a specific version of Homebrew instead of HEAD
- Add timeout for curl download
Apply this diff:
# Check if Homebrew is installed if ! command -v brew &> /dev/null; then echo "Homebrew is not installed. Installing Homebrew..." + + # Known good version and checksum + readonly BREW_VERSION="4.2.4" + readonly BREW_CHECKSUM="valid_sha256_checksum_here" # Replace with actual checksum + # Download script first to allow inspection BREW_SCRIPT=$(mktemp) - if ! curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh -o "$BREW_SCRIPT"; then + if ! curl --max-time 30 -fsSL "https://raw.githubusercontent.com/Homebrew/install/${BREW_VERSION}/install.sh" -o "$BREW_SCRIPT"; then echo "Failed to download Homebrew install script" rm -f "$BREW_SCRIPT" - exit 1 + exit "$E_DOWNLOAD" fi + + # Verify checksum + if ! echo "$BREW_CHECKSUM $BREW_SCRIPT" | sha256sum -c --status; then + echo "Homebrew install script checksum verification failed" + rm -f "$BREW_SCRIPT" + exit "$E_DOWNLOAD" + fi + # Execute the downloaded script if ! /bin/bash "$BREW_SCRIPT"; then echo "Failed to install Homebrew" rm -f "$BREW_SCRIPT" - exit 1 + exit "$E_GENERAL" fi rm -f "$BREW_SCRIPT" + # Add Homebrew to PATH for immediate use - eval "$("$(brew --prefix)/bin/brew" shellenv)" + if ! BREW_PATH="$(brew --prefix)"; then + echo "Failed to get Homebrew prefix" + exit "$E_GENERAL" + fi + if ! eval "$("${BREW_PATH}/bin/brew" shellenv)"; then + echo "Failed to update shell environment" + exit "$E_GENERAL" + fi fi
🧹 Nitpick comments (2)
preinstall.sh (2)
27-29: Improve OS detection robustness.The OS detection could be more robust by handling edge cases.
-# Detect the operating system -OS=$(uname -s) +# Detect the operating system +readonly OS=$(uname -s) + +# Verify supported OS +if [[ ! "$OS" =~ ^(Linux|Darwin)$ ]]; then + echo "Unsupported operating system: $OS" + echo "This script supports only Linux and macOS." + exit "$E_GENERAL" +fi
66-66: Improve final confirmation message.The final message could be more informative by listing the installed packages and their versions.
-echo "All necessary packages are installed." +echo "Installation complete! Installed packages:" +for package in "${VALID_PACKAGES[@]}"; do + if command -v "$package" &> /dev/null; then + version=$("$package" --version 2>/dev/null || echo "version not available") + echo "- $package: $version" + fi +done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
preinstall.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: analyze
- GitHub Check: generate-tag
🔇 Additional comments (1)
preinstall.sh (1)
17-25:⚠️ Potential issueImprove security and robustness of macOS package installation.
The macOS installation function has similar security and robustness concerns.
Apply this diff to improve security and robustness:
install_package_mac() { PACKAGE=$1 + + # Validate package is in allowed list + if [[ ! " ${VALID_PACKAGES[@]} " =~ " ${PACKAGE} " ]]; then + echo "Invalid package name: $PACKAGE" + exit "$E_PACKAGE" + fi + if ! command -v "$PACKAGE" &> /dev/null; then echo "Installing $PACKAGE on macOS..." - brew install $PACKAGE + if ! brew install "$PACKAGE"; then + echo "Failed to install $PACKAGE" + exit "$E_PACKAGE" + fi else echo "$PACKAGE is already installed." fi }Likely invalid or redundant comment.
…/lifinance/contracts into add-preinstall-setup-bash-script
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
preinstall.sh (1)
22-33: Quote variable to prevent word splitting.Add quotes around
$PACKAGEin the command check for consistency with error handling.PACKAGE=$1 - if ! command -v $PACKAGE &> /dev/null; then + if ! command -v "$PACKAGE" &> /dev/null; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
preinstall.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (6)
preinstall.sh (6)
1-4: LGTM!The script initialization is clean and straightforward.
35-41: LGTM!Good practice to check for sudo availability on Linux systems before proceeding.
43-77: LGTM!The implementation follows security best practices:
- Downloads Homebrew script for inspection before execution
- Proper error handling and cleanup
- Clear user messaging for unsupported systems
79-79: LGTM!Clear completion message.
6-20: 🛠️ Refactor suggestionAdd error handling for package installation.
While the apt-get availability check is good, the package installation step needs error handling.
if ! command -v $PACKAGE &> /dev/null; then echo "Installing $PACKAGE on Linux..." sudo apt-get update -y - sudo apt-get install -y $PACKAGE + if ! sudo apt-get install -y "$PACKAGE"; then + echo "Failed to install $PACKAGE" + exit 1 + fiLikely invalid or redundant comment.
45-46: Verify package version compatibility.The script installs the latest versions of
jqandbcwithout version constraints. Please verify that the latest versions are compatible with your project.Also applies to: 71-72
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
preinstall.sh (3)
55-57: Update Linux detection message to reflect multiple package manager support.The message incorrectly states it's using apt package manager, but the script supports both apt-get and dnf.
- echo "Detected Linux. Using apt package manager." + echo "Detected Linux. Checking for supported package managers..."
35-35: Declare PACKAGE as local variable.For consistency with the Linux function and to prevent variable leakage, declare PACKAGE as local.
- PACKAGE=$1 + local PACKAGE=$1
91-91: Enhance completion message with package details.Consider making the success message more informative by listing the installed packages.
-echo "All necessary packages are installed." +echo "Successfully installed required packages: jq, bc"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
preinstall.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (1)
preinstall.sh (1)
50-89: LGTM! Robust OS detection and package management.The implementation includes:
- Proper sudo availability check for Linux
- Secure Homebrew installation with error handling
- Clear error messages for unsupported OS
Which Jira task belongs to this PR?
Why did I implement it this way?
The
scriptMasterscript relies on system-installed packages (bc and jq) for its functionality. Without these dependencies being explicitly installed, users running scriptMaster are likely to encounter various issues beacause both bc and jq are used extensively in the script. This change ensures that the required dependencies are installed on initial yarn install.Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)