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

Use name template variables to customize the name of each resource #234

Merged
merged 3 commits into from
Feb 13, 2023
Merged

Use name template variables to customize the name of each resource #234

merged 3 commits into from
Feb 13, 2023

Conversation

DatsloJRel
Copy link
Contributor

@DatsloJRel DatsloJRel commented Feb 8, 2023

Describe your changes

Added the ability to optionally name resources using variables for name templates. This allows each resource created to have a custom name.

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @DatsloJRel for opening this pr. I like this idea, but a simple var.group_by_vm_instance only provides one alternative naming pattern, I'm thinking of a more flexible way.

My thought is providing several naming template variable to render resource's name, so we can customize the template. e.g.:

var.group_by_vm_instance ? join("-", [var.vm_hostname, host_number, "datadisk", data_disk_number]) : join("-", [var.vm_hostname, "datadisk", host_number, data_disk_number])

This expression implied two different templates: ${vm_hostname}-${host_number}-datadisk-${data_disk_number} && ${vm_hostname}-datadisk-${host_number}-${data_disk_number}. We can also assigned a different template, like ${vm_hostname}-${host_number}-dd-${data_disk_number}.

The problem is, Terraform is lacking of templatestring function like templatefile do. So a possible way is to use multiple replace, like this comment suggested:

replace(replace(replace(var.data_disk_name_template, "${vm_hostname}", var.vm_hostname), "${host_number}", hostnumber), "${data_disk_number}", data_disk_number)

Meanwhile the default value for this var.data_disk_name_template is ${vm_hostname}-datadisk-${host_number}-${data_disk_number}. You can customize your template as you like.

@DatsloJRel DatsloJRel changed the title Add ability to name resources so they are grouped by instance or by resource type Use name template variables to set the name of each resource Feb 9, 2023
Remove the `group_by_vm_instance` and use `*_name_template` variable for each resource to fully customize the name
@DatsloJRel
Copy link
Contributor Author

@lonegunmanb Thanks for the suggestion of using replace(replace(replace(...))) as that is a very nice way of making fully custom resource names. I have updated the code with these changes. Please let me know if this looks acceptable to you, or if you would like some other change associated with it.

In total, there are now 10 new variables for the name templates:

  availability_set_name_template       = "$${vm_hostname}-avset"
  data_disk_name_template              = "$${vm_hostname}-datadisk-$${host_number}-$${data_disk_number}"
  extra_disk_name_template             = "$${vm_hostname}-extradisk-$${host_number}-$${extra_disk_name}"
  network_interface_name_template      = "$${vm_hostname}-nic-$${host_number}"
  network_security_group_name_template = "$${vm_hostname}-nsg"
  public_ip_name_template              = "$${vm_hostname}-pip-$${ip_number}"
  vm_linux_name_template               = "$${vm_hostname}-vmLinux-$${host_number}"
  vm_linux_os_disk_name_template       = "osdisk-$${vm_hostname}-$${host_number}"
  vm_windows_name_template             = "$${vm_hostname}-vmWindows-$${host_number}"
  vm_windows_os_disk_name_template     = "$${vm_hostname}-osdisk-$${host_number}"

It is unfortunate that vm_linux_os_disk_name_template and vm_windows_os_disk_name_template must be different by default, but that is what is existing in the terraform file.

@DatsloJRel DatsloJRel changed the title Use name template variables to set the name of each resource Use name template variables to customize the name of each resource Feb 9, 2023
@DatsloJRel
Copy link
Contributor Author

@lonegunmanb If you would prefer to group the 10 template_name variables together, I had considered naming them with the format of name_template_xxxx, such as name_template_data_disk and name_template_vm_linux. That way, all the name templates would be together in the variables.tf file. I don't know the specific rules for naming, so I will agree with what you think is best.

@lonegunmanb
Copy link
Member

lonegunmanb commented Feb 13, 2023

@lonegunmanb If you would prefer to group the 10 template_name variables together, I had considered naming them with the format of name_template_xxxx, such as name_template_data_disk and name_template_vm_linux. That way, all the name templates would be together in the variables.tf file. I don't know the specific rules for naming, so I will agree with what you think is best.

@DatsloJRel Wow thanks for your careful consideration! I like the name_template_xxx idea, but I have one question: why you choose $${xxx} as template variable instead of using ${xxx}? In terraform's template function, $${xxx} means escape symbol, which leads to ${xxx} as rendered string.

@DatsloJRel
Copy link
Contributor Author

@DatsloJRel Wow thanks for your careful consideration! I like the name_template_xxx idea, but I have one question: why you choose $${xxx} as template variable instead of using ${xxx}? In terraform's template function, $${xxx} means escape symbol, which leads to ${xxx} as rendered string.

@lonegunmanb I will update the PR to name as name_template_xxxx.

As for using $${xxx}, this is required, as Terraform reports an error when trying to use as you suggested.

For example, try this:

variable "name_template_data_disk" {
  description = "The name template for the data disks. The following replacements are automatically made...."
  type        = string
  default     = "${vm_hostname}-datadisk-${host_number}-${data_disk_number}"
}

Results in this error:

│ Error: Variables not allowed
│
│   on variables.tf line 196, in variable "name_template_data_disk":
│  196:   default     = "${vm_hostname}-datadisk-${host_number}-${data_disk_number}"
│
│ Variables may not be used here.

@DatsloJRel
Copy link
Contributor Author

@lonegunmanb There is no rule that we need to use $${xxxx} as the replacement identifier. It could be any string, like <xxxx> or {xxxx} or (xxxx). We just need to use some format in the variables.tf file and in each replace(....) function. The comment that you referenced uses the $${xxx} format, so that is what I used.

@DatsloJRel
Copy link
Contributor Author

@lonegunmanb The PR has been updated for name_template_xxxx now. Let me know if there is anything else.

@DatsloJRel DatsloJRel temporarily deployed to acctests February 13, 2023 07:10 — with GitHub Actions Inactive
Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @DatsloJRel, LGTM! 🚀

@lonegunmanb lonegunmanb merged commit d00d0b0 into Azure:master Feb 13, 2023
@DatsloJRel DatsloJRel deleted the group-by-vm-instance branch February 14, 2023 15:51
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.

2 participants