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

add auto domain to testing framework AWS PT.2 #312

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented May 7, 2024

User description

This adds auto domain to the testing framework for devnets. It continues on #311


PR Type

enhancement


Description

  • Enhanced the testing framework to support configuration of multiple domains (EVM and AUTO).
  • Updated Terraform scripts and Docker compose files to handle new domain configurations.
  • Modified variable definitions and default values to enable the distinction and operation of multiple domains.

Changes walkthrough 📝

Relevant files
Enhancement
bootstrap_node_evm_provisioner.tf
Update EVM and AUTO Domain Configuration in Bootstrap Node Provisioner

testing-framework/ec2/base/bootstrap_node_evm_provisioner.tf

  • Updated environment variable names for domain labels and IDs to
    distinguish between EVM and AUTO domains.
  • Added new environment variables for AUTO domain configuration.
  • +4/-4     
    domain_node_provisioner.tf
    Support Multiple Domains in Domain Node Provisioner           

    testing-framework/ec2/base/domain_node_provisioner.tf

  • Changed domain prefix, label, and ID to support multiple domains (EVM
    and AUTO).
  • +6/-6     
    create_bootstrap_node_evm_compose_file.sh
    Enhance Docker Compose Script for Multiple Domain Support

    testing-framework/ec2/base/scripts/create_bootstrap_node_evm_compose_file.sh

  • Added configurations for AUTO domain alongside existing EVM domain
    settings in Docker compose script.
  • +14/-1   
    create_domain_node_compose_file.sh
    Update Domain Node Docker Compose for Multiple Domains     

    testing-framework/ec2/base/scripts/create_domain_node_compose_file.sh

  • Updated Docker Traefik configurations to handle multiple domains (EVM
    and AUTO).
  • +25/-2   
    Configuration changes
    variables.tf
    Update Domain Prefix to Support Multiple Domains                 

    testing-framework/ec2/base/variables.tf

  • Modified domain-prefix from a single string to a list to support
    multiple domain configurations.
  • +1/-1     
    main.tf
    Configure Network for Multiple Domains                                     

    testing-framework/ec2/network/main.tf

  • Updated domain-prefix to a list containing "nova" and "auto" to enable
    multiple domain configurations.
  • +1/-1     
    variables.tf
    Update Network Variables for Multiple Domain Support         

    testing-framework/ec2/network/variables.tf

  • Updated default values for domain_id and domain_labels to support
    multiple domains.
  • +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @github-actions github-actions bot added the enhancement New feature or request label May 7, 2024
    Copy link

    github-actions bot commented May 7, 2024

    PR Description updated to latest commit (f37f545)

    Copy link

    github-actions bot commented May 7, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple changes across Terraform scripts and Docker compose files, which requires a good understanding of the infrastructure and the specific domain configurations being introduced. The changes are moderate in size but high in impact, requiring careful review to ensure that domain configurations are correctly implemented without affecting existing functionalities.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The use of hardcoded array indices (e.g., domain-labels[0], domain-id[0]) in the Terraform scripts could lead to errors if the array does not have the expected number of elements. This could be mitigated by adding checks or using dynamic data structures that handle variable sizes.

    🔒 Security concerns

    No

    Code feedback:
    relevant filetesting-framework/ec2/base/bootstrap_node_evm_provisioner.tf
    suggestion      

    Consider using a loop or a more dynamic data handling approach instead of hardcoding indices for domain-labels and domain-id. This will make the code more robust and easier to maintain, especially if the number of domains changes in the future. [important]

    relevant line"echo DOMAIN_LABEL_EVM=${var.domain-node-config.domain-labels[0]} >> /home/${var.ssh_user}/subspace/.env",

    relevant filetesting-framework/ec2/base/domain_node_provisioner.tf
    suggestion      

    Implement error handling or checks to ensure that the domain-prefix list contains the expected number of elements before accessing them by index. This prevents runtime errors in environments where the configuration might differ. [important]

    relevant line"echo DOMAIN_PREFIX_EVM=${var.domain-node-config.domain-prefix[0]} >> /home/${var.ssh_user}/subspace/.env",

    relevant filetesting-framework/ec2/base/scripts/create_bootstrap_node_evm_compose_file.sh
    suggestion      

    To improve maintainability, consider abstracting the repeated block of code for domain configuration into a function. This function can then be called with parameters for each domain, reducing code duplication and improving clarity. [medium]

    relevant lineecho ' "--domain-id", "${DOMAIN_ID_EVM}",'

    relevant filetesting-framework/ec2/base/scripts/create_domain_node_compose_file.sh
    suggestion      

    Use a loop to generate Traefik router configurations dynamically based on domain information. This approach reduces the risk of errors and simplifies modifications when adding or removing domains. [medium]

    relevant line- "traefik.http.routers.archival-node.rule=Host(\`\${DOMAIN_PREFIX_EVM}-\${DOMAIN_ID_EVM}.\${NETWORK_NAME}.subspace.network\`) && Path(\`/ws\`)"

    Copy link

    github-actions bot commented May 7, 2024

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Best practice
    Replace hyphens with underscores in variable names for better consistency and parsing.

    Replace the hyphen in the variable name with an underscore for consistency and to avoid
    potential parsing issues.

    testing-framework/ec2/base/bootstrap_node_evm_provisioner.tf [153-156]

    -"echo DOMAIN_LABEL_EVM=${var.domain-node-config.domain-labels[0]} >> /home/${var.ssh_user}/subspace/.env",
    -"echo DOMAIN_ID_EVM=${var.domain-node-config.domain-id[0]} >> /home/${var.ssh_user}/subspace/.env",
    -"echo DOMAIN_LABEL_AUTO=${var.domain-node-config.domain-labels[1]} >> /home/${var.ssh_user}/subspace/.env",
    -"echo DOMAIN_ID_AUTO=${var.domain-node-config.domain-id[1]} >> /home/${var.ssh_user}/subspace/.env",
    +"echo DOMAIN_LABEL_EVM=${var.domain_node_config.domain_labels[0]} >> /home/${var.ssh_user}/subspace/.env",
    +"echo DOMAIN_ID_EVM=${var.domain_node_config.domain_id[0]} >> /home/${var.ssh_user}/subspace/.env",
    +"echo DOMAIN_LABEL_AUTO=${var.domain_node_config.domain_labels[1]} >> /home/${var.ssh_user}/subspace/.env",
    +"echo DOMAIN_ID_AUTO=${var.domain_node_config.domain_id[1]} >> /home/${var.ssh_user}/subspace/.env",
     
    Enhancement
    Add error handling for address retrieval in script commands.

    Add error handling to check if the sed command successfully retrieves the address before
    appending it to the docker-compose file.

    testing-framework/ec2/base/scripts/create_bootstrap_node_evm_compose_file.sh [165-167]

     addr=$(sed -nr "s/NODE_${i}_OPERATOR_MULTI_ADDR_TCP=//p" ~/subspace/node_keys.txt)
    -echo "      \"--reserved-nodes\", \"${addr}\"," >> ~/subspace/subspace/docker-compose.yml
    -echo "      \"--bootstrap-nodes\", \"${addr}\"," >> ~/subspace/subspace/docker-compose.yml
    +if [ -n "$addr" ]; then
    +  echo "      \"--reserved-nodes\", \"${addr}\"," >> ~/subspace/subspace/docker-compose.yml
    +  echo "      \"--bootstrap-nodes\", \"${addr}\"," >> ~/subspace/subspace/docker-compose.yml
    +else
    +  echo "Error: Unable to retrieve address for NODE_${i}"
    +fi
     
    Use a map for domain configurations to simplify data management.

    Consider using a map instead of separate lists for domain configurations to simplify data
    management and access.

    testing-framework/ec2/base/variables.tf [88]

    -domain-prefix      = list(string)
    +domain-config = map(object({
    +  prefix = string
    +  label = string
    +  id = number
    +}))
     
    Maintainability
    Use a loop to generate domain configuration echo statements to reduce code redundancy.

    Consider using a loop to generate these echo statements to reduce redundancy and improve
    maintainability.

    testing-framework/ec2/base/domain_node_provisioner.tf [162-167]

    -"echo DOMAIN_PREFIX_EVM=${var.domain-node-config.domain-prefix[0]} >> /home/${var.ssh_user}/subspace/.env",
    -"echo DOMAIN_PREFIX_AUTO=${var.domain-node-config.domain-prefix[1]} >> /home/${var.ssh_user}/subspace/.env",
    -"echo DOMAIN_LABEL_EVM=${var.domain-node-config.domain-labels[0]} >> /home/${var.ssh_user}/subspace/.env",
    -"echo DOMAIN_ID_EVM=${var.domain-node-config.domain-id[0]} >> /home/${var.ssh_user}/subspace/.env",
    -"echo DOMAIN_LABEL_AUTO=${var.domain-node-config.domain-labels[1]} >> /home/${var.ssh_user}/subspace/.env",
    -"echo DOMAIN_ID_AUTO=${var.domain-node-config.domain-id[1]} >> /home/${var.ssh_user}/subspace/.env",
    +for idx in {0..1}
    +do
    +  echo "echo DOMAIN_PREFIX_${var.domain-node-config.domain-prefix[idx]} >> /home/${var.ssh_user}/subspace/.env",
    +  echo "echo DOMAIN_LABEL_${var.domain-node-config.domain-labels[idx]} >> /home/${var.ssh_user}/subspace/.env",
    +  echo "echo DOMAIN_ID_${var.domain-node-config.domain-id[idx]} >> /home/${var.ssh_user}/subspace/.env",
    +done
     
    Use a loop to generate Traefik router configurations for different domains.

    Use a loop to generate Traefik router configurations to reduce redundancy and improve
    maintainability.

    testing-framework/ec2/base/scripts/create_domain_node_compose_file.sh [63-68]

    -- "traefik.http.routers.archival-node.rule=Host(\`\${DOMAIN_PREFIX_EVM}-\${DOMAIN_ID_EVM}.\${NETWORK_NAME}.subspace.network\`) && Path(\`/ws\`)"
    -- "traefik.http.routers.archival-node-auto.rule=Host(\`\${DOMAIN_PREFIX_AUTO}-\${DOMAIN_ID_AUTO}.\${NETWORK_NAME}.subspace.network\`) && Path(\`/ws\`)"
    +domains=("EVM" "AUTO")
    +for domain in "${domains[@]}"
    +do
    +  echo "- \"traefik.http.routers.archival-node-$domain.rule=Host(\`\${DOMAIN_PREFIX_$domain}-\${DOMAIN_ID_$domain}.\${NETWORK_NAME}.subspace.network\`) && Path(\`/ws\`)\""
    +done
     

    decouple domains to 2 separate ones
    
    - add autoid domain
    @DaMandal0rian DaMandal0rian force-pushed the extend-domain-testing-framework branch from 8f69b76 to 0bec6ea Compare May 23, 2024 18:08
    @DaMandal0rian DaMandal0rian merged commit 4eaab1e into main Jul 19, 2024
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the extend-domain-testing-framework branch July 19, 2024 12:20
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants