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

changes to scipts and tag names #374

Merged
merged 3 commits into from
Nov 6, 2024
Merged

changes to scipts and tag names #374

merged 3 commits into from
Nov 6, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented Nov 6, 2024

User description

  • change tag names to be uniform and standard
  • make launch script support more bootnodes

PR Type

enhancement


Description

  • Standardized the naming of tags from node-tag to docker-tag across multiple Terraform configuration files and scripts.
  • Enhanced the Python script to support and manage multiple bootstrap nodes, improving scalability.
  • Updated the TOML configuration to include multiple bootstrap nodes, reflecting the changes in the Python script.
  • Improved consistency in environment variable naming in Docker Compose files.

Changes walkthrough 📝

Relevant files
Enhancement
9 files
main.tf
Standardize tag naming in node configurations                       

resources/leaseweb/main.tf

  • Changed node-tag to docker-tag for consistency.
  • Updated multiple node configurations.
  • +5/-5     
    bootstrap_node_domain_provisioner.tf
    Update environment variable naming for bootstrap nodes     

    templates/terraform/leaseweb/bootstrap_node_domain_provisioner.tf

    • Changed NODE_TAG to DOCKER_TAG in .env file creation.
    +1/-1     
    bootstrap_node_provisioner.tf
    Update environment variable naming for bootstrap nodes     

    templates/terraform/leaseweb/bootstrap_node_provisioner.tf

    • Changed NODE_TAG to DOCKER_TAG in .env file creation.
    +1/-1     
    domain_node_provisioner.tf
    Update environment variable naming for domain nodes           

    templates/terraform/leaseweb/domain_node_provisioner.tf

    • Changed NODE_TAG to DOCKER_TAG in .env file creation.
    +1/-1     
    farmer_node_provisioner.tf
    Update environment variable naming for farmer nodes           

    templates/terraform/leaseweb/farmer_node_provisioner.tf

    • Changed NODE_TAG to DOCKER_TAG in .env file creation.
    +1/-1     
    node_provisioner.tf
    Update environment variable naming for nodes                         

    templates/terraform/leaseweb/node_provisioner.tf

    • Changed NODE_TAG to DOCKER_TAG in .env file creation.
    +1/-1     
    variables.tf
    Standardize tag naming in variable definitions                     

    templates/terraform/leaseweb/variables.tf

    • Changed node-tag to docker-tag in variable definitions.
    +5/-5     
    manage_subspace.py
    Support multiple bootstrap nodes in management script       

    scripts/launch-nodes/manage_subspace.py

  • Modified to handle multiple bootstrap nodes.
  • Updated logging and error handling for bootstrap nodes.
  • +13/-13 
    nodes.toml
    Add support for multiple bootstrap nodes in configuration

    scripts/launch-nodes/nodes.toml

  • Added multiple bootstrap nodes.
  • Updated user configuration for nodes.
  • +21/-5   

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

    Copy link

    github-actions bot commented Nov 6, 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

    Tag Replacement
    The replacement of 'node-tag' with 'docker-tag' across various configurations ensures consistency but needs to be checked for correct application in all instances.

    Environment Configuration
    The addition of 'DOCKER_TAG' to the environment setup script should be validated to ensure it correctly reflects the new tagging convention without disrupting the deployment process.

    Script Logic Change
    The modification to handle multiple bootstrap nodes instead of a single node could affect the script's execution flow and error handling capabilities.

    Copy link

    github-actions bot commented Nov 6, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Prevent potential NameError by checking if client is defined before attempting to close it

    Ensure that the client variable is defined before attempting to close it in the
    finally block to avoid a potential NameError if the ssh_connect function fails
    before client is assigned.

    scripts/launch-nodes/manage_subspace.py [297-298]

    -if client:
    +if 'client' in locals():
         client.close()
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential bug where the 'client' variable might not be defined before it is used, which could lead to a NameError if the ssh_connect function fails. This is a critical fix for robust error handling in the script.

    8

    @DaMandal0rian DaMandal0rian merged commit 5458cdf into main Nov 6, 2024
    2 checks passed
    @DaMandal0rian DaMandal0rian deleted the mainnet-launch branch November 6, 2024 13:03
    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.

    1 participant