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: Update subnet configuration in main.tf to utilize the 'metal_subnet' variable. #75

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

codinja1188
Copy link
Contributor

Refactor subnet configuration and comments

This commit refactors the subnet configuration in the 'main.tf' file to utilize the 'metal_subnet' variable instead of hardcoding the subnet value. Previously, an arbitrary subnet '192.168.100.0/22' was used directly within the file. By introducing the 'metal_subnet' variable, the configuration becomes more flexible and easier to manage, allowing users to specify their desired subnet for the Nutanix cluster.

Additionally, this commit removes redundant comments related to subnet configuration that were no longer necessary after the refactor. These comments provided guidance on selecting a subnet, which is now handled through the 'metal_subnet' variable, making the comments obsolete.

Overall, this refactor improves the maintainability and flexibility of the Terraform configuration for deploying Nutanix clusters on Equinix Metal infrastructure.

variables.tf Outdated
@@ -31,6 +31,12 @@ variable "metal_project_id" {
EOT
}

variable "metal_subnet" {
Copy link
Member

Choose a reason for hiding this comment

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

This PR seems fine except for the name of the variable. The name should match what the CIDR is used for since it is defined in both "metal" (APIs), the basion, and Nutanix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to mention any restrictions on the private subnet that is used or do we expect any RFC1918 subnet to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is recomended variable name ?

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect any to work. @cprivitere initially tested with a different CIDR and used this CIDR to match the guides.

Copy link
Member

Choose a reason for hiding this comment

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

@codinja1188 cluster_cidr ? (something that is not specifically Metal nor Nutanix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@displague , @cprivitere

will go with RF1918, OR RFC6598, check the PR and approve release it, It's really help in VM migration As I mentioned #68 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I'll accept metal_subnet as-is.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I can't merge this because it does not pass the linter.

 variable "metal_subnet" {
-  type = string
-  default = "192.168.100.0/22"
+  type        = string
+  default     = "192.168.100.0/22"
   description = "nutanix cluster subnet"
 }

@codinja1188 If you take this opportunity to rename the field to cluster_subnet, be sure to capture the rename everywhere metal_subnet is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@displague,

updated in latest commit

@displague displague merged commit 1ceb2de into equinix-labs:main Jun 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants