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

bug fixes subql #364

Merged
merged 1 commit into from
Nov 1, 2024
Merged

bug fixes subql #364

merged 1 commit into from
Nov 1, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented Nov 1, 2024

PR Type

Bug fix, Enhancement


Description

  • Enhanced permission handling by adding sudo to directory creation commands in Terraform scripts.
  • Streamlined subql stack setup by repeating necessary commands directly in Terraform scripts.
  • Removed redundant 'Installation Complete' echo statements to clean up script output.
  • Simplified the install_subql_stack.sh script by removing export and build commands, moving them to Terraform scripts.

Changes walkthrough 📝

Relevant files
Enhancement
bootstrap_nova_subql_provisioner.tf
Improve permission handling and streamline subql stack setup

templates/terraform/subql/base/bootstrap_nova_subql_provisioner.tf

  • Added sudo to directory creation commands for permission handling.
  • Repeated installation and setup commands for subql stack.
  • Removed redundant 'Installation Complete' echo.
  • +22/-5   
    bootstrap_subql_provisioner.tf
    Enhance permission handling and streamline subql stack setup

    templates/terraform/subql/base/bootstrap_subql_provisioner.tf

  • Added sudo to directory creation commands for permission handling.
  • Repeated installation and setup commands for subql stack.
  • Removed redundant 'Installation Complete' echo.
  • +22/-9   
    install_subql_stack.sh
    Simplify subql stack installation script                                 

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

    • Removed export and build commands from script.
    +0/-12   

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

    @DaMandal0rian DaMandal0rian changed the title bug fixes bug fixes subql Nov 1, 2024
    Copy link

    github-actions bot commented Nov 1, 2024

    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

    Permissions Handling
    The use of 'sudo' for directory creation might indicate permission issues that could be better handled through proper user and group management.

    Script Execution
    The script execution commands are repeated in multiple sections, which could be refactored into a function or script to avoid redundancy and ease maintenance.

    Copy link

    github-actions bot commented Nov 1, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Simplify directory creation by combining multiple mkdir commands into one

    Combine the multiple sudo mkdir -p commands into a single command using curly braces
    to create directories and subdirectories in one line. This will reduce the number of
    remote commands executed and simplify the script.

    templates/terraform/subql/base/bootstrap_nova_subql_provisioner.tf [94-97]

    -"sudo mkdir -p /home/${var.ssh_user}/subql",
    -"sudo mkdir -p /home/${var.ssh_user}/subql/postgresql",
    -"sudo mkdir -p /home/${var.ssh_user}/subql/postgresql/conf",
    -"sudo mkdir -p /home/${var.ssh_user}/subql/postgresql/data"
    +"sudo mkdir -p /home/${var.ssh_user}/subql /home/${var.ssh_user}/subql/postgresql /home/${var.ssh_user}/subql/postgresql/conf /home/${var.ssh_user}/subql/postgresql/data"
    Suggestion importance[1-10]: 7

    Why: Combining multiple mkdir commands into one reduces the number of remote commands executed, simplifying the script and potentially improving performance. This is a valid enhancement for maintainability and efficiency.

    7
    Maintainability
    Streamline environment variable updates in the .env file to enhance script clarity and reliability

    Consolidate the grep -q and sed -i or echo commands into a more efficient script
    flow by checking and updating the .env file in a single operation per variable to
    reduce complexity and potential errors.

    templates/terraform/subql/base/bootstrap_nova_subql_provisioner.tf [166-170]

    -"grep -q '^NR_API_KEY=' /home/${var.ssh_user}/astral/.env && sed -i 's/^NR_API_KEY=.*/NR_API_KEY=${var.nr_api_key}/' /home/${var.ssh_user}/astral/.env || echo NR_API_KEY=${var.nr_api_key} >> /home/${var.ssh_user}/astral/.env",
    +"if grep -q '^NR_API_KEY=' /home/${var.ssh_user}/astral/.env; then sed -i 's/^NR_API_KEY=.*/NR_API_KEY=${var.nr_api_key}/' /home/${var.ssh_user}/astral/.env; else echo NR_API_KEY=${var.nr_api_key} >> /home/${var.ssh_user}/astral/.env; fi",
     ...
    -"grep -q '^HASURA_GRAPHQL_JWT_SECRET=' /home/${var.ssh_user}/astral/.env && sed -i 's|^HASURA_GRAPHQL_JWT_SECRET=.*|HASURA_GRAPHQL_JWT_SECRET={\"type\":\"HS256\",\"key\":\"${var.hasura_graphql_jwt_secret}\"}|' /home/${var.ssh_user}/astral/.env || echo HASURA_GRAPHQL_JWT_SECRET={\"type\":\"HS256\",\"key\":\"${var.hasura_graphql_jwt_secret}\"} >> /home/${var.ssh_user}/astral/.env"
    +"if grep -q '^HASURA_GRAPHQL_JWT_SECRET=' /home/${var.ssh_user}/astral/.env; then sed -i 's|^HASURA_GRAPHQL_JWT_SECRET=.*|HASURA_GRAPHQL_JWT_SECRET={\"type\":\"HS256\",\"key\":\"${var.hasura_graphql_jwt_secret}\"}|' /home/${var.ssh_user}/astral/.env; else echo HASURA_GRAPHQL_JWT_SECRET={\"type\":\"HS256\",\"key\":\"${var.hasura_graphql_jwt_secret}\"} >> /home/${var.ssh_user}/astral/.env; fi"
    Suggestion importance[1-10]: 7

    Why: Consolidating the grep, sed, and echo commands into a single operation per variable reduces complexity and potential errors, enhancing maintainability and clarity of the script. This is a beneficial improvement for the codebase.

    7
    Best practice
    Ensure directory permissions are correctly set after creation to avoid access issues

    Ensure that the sudo chown and sudo chmod commands are executed after all
    directories are created to avoid potential permission issues during directory
    creation.

    templates/terraform/subql/base/bootstrap_nova_subql_provisioner.tf [94-98]

    -"sudo mkdir -p /home/${var.ssh_user}/subql",
    -"sudo mkdir -p /home/${var.ssh_user}/subql/postgresql",
    -"sudo mkdir -p /home/${var.ssh_user}/subql/postgresql/conf",
    -"sudo mkdir -p /home/${var.ssh_user}/subql/postgresql/data",
    -"sudo chown -R ${var.ssh_user}:${var.ssh_user} /home/${var.ssh_user}/subql/ && sudo chmod -R 750 /home/${var.ssh_user}/subql/"
    +"sudo mkdir -p /home/${var.ssh_user}/subql /home/${var.ssh_user}/subql/postgresql /home/${var.ssh_user}/subql/postgresql/conf /home/${var.ssh_user}/subql/postgresql/data && sudo chown -R ${var.ssh_user}:${var.ssh_user} /home/${var.ssh_user}/subql/ && sudo chmod -R 750 /home/${var.ssh_user}/subql/"
    Suggestion importance[1-10]: 6

    Why: The suggestion to ensure chown and chmod commands are executed after all directories are created is a good practice to prevent permission issues. However, the improved code combines all commands into one line, which may not be necessary for this purpose.

    6
    Performance
    Eliminate redundant script execution to streamline operations

    Replace the repeated chmod +x and bash commands for the install_subql_stack.sh
    script with a single instance at the beginning of the provisioning to avoid
    redundancy and improve script efficiency.

    templates/terraform/subql/base/bootstrap_nova_subql_provisioner.tf [173-174]

    -"chmod +x /home/${var.ssh_user}/subql/install_subql_stack.sh",
    -"bash /home/${var.ssh_user}/subql/install_subql_stack.sh",
    -...
     "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]: 5

    Why: The suggestion to remove redundant chmod and bash commands is valid for improving script efficiency. However, the suggestion does not fully address the context of the PR, where these commands are executed in different sections.

    5

    Copy link

    @marc-aurele-besner marc-aurele-besner left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM

    @DaMandal0rian DaMandal0rian merged commit a84e6c9 into main Nov 1, 2024
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the hotfix/subql-bug-fix branch November 1, 2024 14:43
    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