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

module refactor for networks #357

Merged
merged 4 commits into from
Oct 28, 2024
Merged

module refactor for networks #357

merged 4 commits into from
Oct 28, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented Oct 28, 2024

User description

  • remove old networks gemini-3f and gemini-3g
  • make domains more configurable and reusable
  • fix variables and naming
  • improve DNS handling of domains

PR Type

Enhancement


Description

  • Refactored Terraform configurations to replace evm-node-config and autoid-node-config with domain-node-config.
  • Enhanced DNS handling by introducing local mappings for nova and autoid instances.
  • Updated AWS instance configurations, including instance count and type settings.
  • Modified environment variable setups across multiple provisioner files.
  • Removed outdated configurations for autoid nodes and improved domain node settings.

Changes walkthrough 📝

Relevant files
Enhancement
15 files
instances.tf
Update AWS instance configurations and domain node settings

templates/terraform/network-primitives/instances.tf

  • Reintroduced and updated configuration for aws_instance resources.
  • Changed instance count and type configurations to use
    domain-node-config.
  • Updated EBS block device configurations.
  • Modified SSH connection setup and provisioner scripts.
  • +197/-197
    dns.tf
    Enhance DNS configuration with instance mappings                 

    templates/terraform/network-primitives-archive/gemini-3h/dns.tf

  • Added local mappings for nova and autoid instances.
  • Updated Cloudflare record resources to use for_each.
  • Changed domain prefix and IP address configurations.
  • +50/-30 
    autoid_node_provisioner.tf
    Refactor autoid node provisioner to use domain config       

    templates/terraform/network-primitives-archive/gemini-3h/autoid_node_provisioner.tf

  • Replaced autoid-node-config with domain-node-config.
  • Updated triggers and connection settings.
  • Modified environment variable setup for nodes.
  • +11/-12 
    autoid_node_provisioner.tf
    Refactor autoid node provisioner to use domain config       

    templates/terraform/network-primitives/autoid_node_provisioner.tf

  • Replaced autoid-node-config with domain-node-config.
  • Updated triggers and connection settings.
  • Modified environment variable setup for nodes.
  • +11/-12 
    evm_node_provisioner.tf
    Refactor EVM node provisioner to use domain config             

    templates/terraform/network-primitives/evm_node_provisioner.tf

  • Replaced evm-node-config with domain-node-config.
  • Updated triggers and connection settings.
  • Modified environment variable setup for nodes.
  • +11/-12 
    dns.tf
    Enhance DNS configuration with instance mappings                 

    templates/terraform/network-primitives/dns.tf

  • Added local mappings for nova and autoid instances.
  • Updated Cloudflare record resources to use for_each.
  • Changed domain prefix and IP address configurations.
  • +42/-22 
    nova_squid_node_provisioner.tf
    Update nova squid node provisioner with domain settings   

    templates/terraform/network-primitives-archive/gemini-3h/nova_squid_node_provisioner.tf

  • Updated domain prefix and label configurations.
  • Modified environment variable setup for nodes.
  • +4/-5     
    nova_indexer_node_provisioner.tf
    Update nova indexer node provisioner with domain settings

    templates/terraform/network-primitives/nova_indexer_node_provisioner.tf

  • Updated domain prefix and label configurations.
  • Modified environment variable setup for nodes.
  • +4/-5     
    variables.tf
    Refactor variable configurations for domain nodes               

    templates/terraform/network-primitives/variables.tf

  • Renamed evm-node-config to domain-node-config.
  • Removed autoid-node-config.
  • Updated instance count and type configurations.
  • +13/-49 
    bootstrap_node_autoid_provisioner.tf
    Update bootstrap node autoid provisioner with domain config

    templates/terraform/network-primitives/bootstrap_node_autoid_provisioner.tf

  • Replaced evm-node-config with domain-node-config.
  • Updated environment variable setup for nodes.
  • +3/-4     
    bootstrap_node_evm_provisioner.tf
    Update bootstrap node EVM provisioner with domain config 

    templates/terraform/network-primitives/bootstrap_node_evm_provisioner.tf

  • Replaced evm-node-config with domain-node-config.
  • Updated environment variable setup for nodes.
  • +3/-4     
    main.tf
    Update main configuration for gemini-3h module                     

    resources/gemini-3h/main.tf

  • Updated domain prefix to include autoid.
  • Removed autoid-node-config.
  • +2/-20   
    variables.tf
    Fix and update variable configurations for gemini-3h         

    resources/gemini-3h/variables.tf

  • Fixed syntax error in domain labels default value.
  • Updated instance type and count configurations.
  • +16/-16 
    variables.tf
    Fix and update variable configurations for taurus               

    resources/taurus/variables.tf

  • Fixed syntax error in domain labels default value.
  • Updated instance type and count configurations.
  • +16/-16 
    main.tf
    Update main configuration for taurus module                           

    resources/taurus/main.tf

  • Updated domain prefix to include autoid.
  • Removed autoid-node-config.
  • +2/-20   

    💡 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: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Configuration Changes
    Significant changes to instance configurations and removal of old node configurations suggest a thorough validation is needed to ensure all interdependencies and new configurations are correctly set up.

    Default Values
    Changes in default values for various configurations such as instance types and counts could impact the overall resource allocation and cost. Review these changes for potential impacts on budget and resource utilization.

    DNS Configuration
    The introduction of explicit mappings for nova and autoid instances in DNS configurations requires validation to ensure correct DNS resolution and no conflicts or overlaps with existing configurations.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Safely handle division in instance count to ensure it results in an integer

    Ensure that the division operation for instance-count in domain-node-config is
    safely handled to avoid a non-integer result which Terraform cannot process as a
    count.

    resources/gemini-3h/main.tf [384]

    -count = length(var.aws_region) * (var.domain-node-config.instance-count / 2)
    +count = length(var.aws_region) * floor(var.domain-node-config.instance-count / 2)
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a potential bug where the division operation could result in a non-integer value, which Terraform cannot process as a count. Using floor ensures the result is an integer, thus preventing runtime errors.

    9
    Ensure integer division is used for calculating instance counts

    Ensure that the division operation in the instance-count calculation is explicitly
    handled for integer division to avoid unexpected behavior when the count is not
    evenly divisible.

    templates/terraform/network-primitives/bootstrap_node_evm_provisioner.tf [384]

    -count = length(var.aws_region) * (var.domain-node-config.instance-count / 2)
    +count = length(var.aws_region) * (var.domain_node_config.instance_count // 2)
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a potential bug where floating-point division could lead to incorrect instance counts. By using integer division, the code ensures that the instance count is calculated accurately, preventing unexpected behavior.

    9
    Ensure all instances are accounted for in split calculations

    Verify that the instance_split calculation correctly handles cases where
    instance-count is odd, potentially using a ceiling function to ensure all instances
    are accounted for.

    templates/terraform/network-primitives/bootstrap_node_provisioner.tf [7]

    -instance_split = var.domain-node-config.instance-count / 2
    +instance_split = ceil(var.domain_node_config.instance_count / 2.0)
    Suggestion importance[1-10]: 8

    Why: The suggestion highlights a potential issue with handling odd instance counts. Using a ceiling function ensures that all instances are accounted for, which is crucial for accurate resource allocation.

    8
    Possible issue
    Ensure correct instance splitting when the count is odd

    Verify that the instance_split calculation in the locals block correctly handles
    cases where instance-count is odd, to ensure a proper split of instances between
    nova and autoid.

    resources/gemini-3h/main.tf [7]

    -instance_split = var.domain-node-config.instance-count / 2
    +instance_split = floor(var.domain-node-config.instance-count / 2)
    Suggestion importance[1-10]: 8

    Why: This suggestion is important as it ensures that the instance splitting logic correctly handles odd numbers, preventing potential misallocation of instances between nova and autoid.

    8
    Best practice
    Improve variable naming consistency by using underscores instead of hyphens

    Replace the hyphen in the variable name domain-node-config with an underscore to
    maintain consistency and avoid potential parsing errors.

    templates/terraform/network-primitives/bootstrap_node_evm_provisioner.tf [150]

    -"echo DOMAIN_LABEL=${var.domain-node-config.domain-labels[0]} >> /home/${var.ssh_user}/subspace/.env"
    +"echo DOMAIN_LABEL=${var.domain_node_config.domain_labels[0]} >> /home/${var.ssh_user}/subspace/.env"
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential parsing issue by replacing hyphens with underscores in variable names, which is a common best practice in Terraform to avoid errors. This change enhances consistency and reduces the risk of syntax errors.

    8
    Add error handling for potentially empty lists in Terraform loops

    Add error handling for the for_each directive in Terraform to manage potential empty
    lists or null values which could cause runtime errors.

    templates/terraform/network-primitives/dns.tf [34]

    -for_each = local.nova_instances
    +for_each = length(local.nova_instances) > 0 ? local.nova_instances : {}
    Suggestion importance[1-10]: 7

    Why: The suggestion improves robustness by adding error handling for empty lists in Terraform loops. This prevents runtime errors and ensures that the code can handle edge cases gracefully.

    7
    Enhancement
    Validate thread-pool-size to ensure it contains valid numeric values

    Ensure that the new thread-pool-size variable in farmer-node-config is appropriately
    validated to avoid misconfiguration that could lead to performance issues.

    resources/gemini-3h/main.tf [135]

    -thread-pool-size = var.thread_pool_size
    +thread-pool-size = can(regex("^[0-9]+$", var.thread_pool_size)) ? var.thread_pool_size : "default_value"
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances the code by adding validation for the thread-pool-size variable, which helps prevent misconfiguration and potential performance issues. However, the use of a default value without context may not be ideal.

    7

    @DaMandal0rian DaMandal0rian merged commit ddc64c9 into main Oct 28, 2024
    2 checks passed
    @DaMandal0rian DaMandal0rian deleted the hotfix/modules branch October 28, 2024 16:28
    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