Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/subql launch script improve #362

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented Oct 31, 2024

PR Type

enhancement, bug fix


Description

  • Remove parts of script that manages tmux sessions since it's not stable with terraform.
  • run stack with combination of yarn and docker compose commands
  • Enhanced the setup process by adding error handling and switching to nvm for Node.js installation.
  • Corrected variable names and improved the .env file creation process to prevent duplicate entries.
  • Updated the Terraform provisioners to include the new run script and adjusted script destinations for clarity.

Changes walkthrough 📝

Relevant files
Enhancement
bootstrap_nova_subql_provisioner.tf
Enhance and fix subql node setup and script execution       

templates/terraform/subql/base/bootstrap_nova_subql_provisioner.tf

  • Added provisioner for copying run_subql_stack.sh script.
  • Corrected variable names for node IP and hostname settings.
  • Updated .env file creation to remove existing entries before
    appending.
  • Changed execution of scripts to include new run_subql_stack.sh.
  • +37/-16 
    bootstrap_subql_provisioner.tf
    Improve subql node setup with new run script                         

    templates/terraform/subql/base/bootstrap_subql_provisioner.tf

  • Added provisioner for copying run_subql_stack.sh script.
  • Updated script destinations for clarity.
  • Changed execution of scripts to include new run_subql_stack.sh.
  • +36/-15 
    install_subql_stack.sh
    Refactor subql stack installation script for robustness   

    templates/terraform/subql/base/scripts/install_subql_stack.sh

  • Added error handling with set -e.
  • Switched to using nvm for Node.js installation.
  • Removed old tmux session commands.
  • Added yarn installation check and installation.
  • +25/-34 
    run_subql_stack.sh
    Add new script for running subql stack with tmux                 

    templates/terraform/subql/base/scripts/run_subql_stack.sh

  • Introduced new script to manage subql stack execution.
  • Added functions for tmux session management.
  • Implemented waiting mechanism for indexer sync.
  • Automated starting of indexers, metadata, and Hasura console.
  • +105/-0 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication
    The script handling logic for setting up and running the subql stack is duplicated across multiple resources for different node setups. Consider refactoring to reduce redundancy and improve maintainability.

    Error Handling
    The script exits upon any error without specific error messages related to the context, which might make debugging more difficult. Consider adding more descriptive error handling and recovery paths.

    Resource Management
    The script forcefully kills existing tmux sessions which might lead to unexpected behavior or data loss. Consider checking and handling existing sessions more gracefully.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Verify and optimize the use of chmod +x for script execution

    Verify that the chmod +x command is necessary before running scripts, and consider
    setting executable permissions at the time of file transfer.

    templates/terraform/subql/base/bootstrap_nova_subql_provisioner.tf [168-169]

    -"chmod +x /home/${var.ssh_user}/subql/install_subql_stack.sh",
     "bash /home/${var.ssh_user}/subql/install_subql_stack.sh"
    Suggestion importance[1-10]: 7

    Why: The suggestion to verify the necessity of chmod +x and consider setting executable permissions during file transfer is valid. It could optimize the script execution process and reduce redundancy, enhancing efficiency.

    7
    Best practice
    Add verification steps after installing Node.js and npm to ensure their successful setup

    Consider verifying the successful installation of Node.js and npm after using nvm to
    install them, to ensure the script does not proceed with further steps if this
    critical installation fails.

    templates/terraform/subql/base/scripts/install_subql_stack.sh [29-35]

    -nvm install --lts
    -nvm use --lts
    -node -v
    -npm -v
    +nvm install --lts && nvm use --lts && node -v && npm -v
    Suggestion importance[1-10]: 6

    Why: The suggestion to combine installation and verification commands ensures that the script halts if Node.js or npm installation fails, preventing subsequent errors. This enhances the robustness of the script.

    6

    @DaMandal0rian DaMandal0rian force-pushed the feat/subql-launch-script-improve branch 2 times, most recently from 2dd8fe6 to c53f22a Compare October 31, 2024 22:35
    fix variables
    
    remove run script since its not stable
    
    add env variables
    @DaMandal0rian DaMandal0rian force-pushed the feat/subql-launch-script-improve branch from c53f22a to f9c0227 Compare November 1, 2024 01:05
    @DaMandal0rian DaMandal0rian force-pushed the feat/subql-launch-script-improve branch from b53caca to b0a1517 Compare November 1, 2024 10:54
    @DaMandal0rian DaMandal0rian merged commit 5058e18 into main Nov 1, 2024
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the feat/subql-launch-script-improve branch November 1, 2024 10:54
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants