feat(MAJOR): full refactor of modules and examples #251
Conversation
Co-authored-by: Sebastian Czech <[email protected]>
| } | ||
|
|
||
| variable "img_sku" { | ||
| description = "VM-Series SKU - list available with `az vm image list -o table --all --publisher paloaltonetworks`" | ||
| default = "bundle2" | ||
| default = "byol" |
| @@ -1,3 +1,9 @@ | |||
| variable "name_prefix" { | |||
There was a problem hiding this comment.
The need for adding a name_prefix at module level can be mitigated in the "glue code", please refer to this example: https://github.com/PaloAltoNetworks/terraform-google-vmseries-modules/pull/182/files#diff-8fdcb1bfa2b32baa79c1716fd9558e3ef35f1ad0a13bc71267300689ddf9e206R64
| location = var.location | ||
|
|
||
| workspace_mode = try(var.application_insights.workspace_mode, null) | ||
| workspace_name = try(var.application_insights.workspace_name, "${var.name_prefix}${each.key}-wrkspc") |
There was a problem hiding this comment.
A static string is part of this name. Also in a few other AI related parts of the code:
https://github.com/PaloAltoNetworks/terraform-azurerm-vmseries-modules/pull/251/files#diff-9e1c24bf63ecdd30b06e538a06f020db09beae8d112e53baed683c79620f0cbfR127
https://github.com/PaloAltoNetworks/terraform-azurerm-vmseries-modules/pull/251/files#diff-9e1c24bf63ecdd30b06e538a06f020db09beae8d112e53baed683c79620f0cbfR166
There was a problem hiding this comment.
comment same as here
| @@ -3,13 +3,22 @@ resource "azurerm_public_ip" "this" { | |||
|
|
|||
| location = var.location | |||
| resource_group_name = var.resource_group_name | |||
| name = each.value.name | |||
| name = "${each.value.name}-pip" | |||
There was a problem hiding this comment.
I can see this suffix being added throughout the PR, in different modules. I think we should avoid any static suffixes in resource names whenever possible.
There was a problem hiding this comment.
I do not see a place where they could harm the code or usability. Especially that these are only names, we operate on keys everywhere, so the code does not depend on them.
And it is more readable when you look in Azure portal. Keeping the naming schema, where each related resource get's a prefix in the form of the main resource + some suffix that usually describes a type of a resource groups the related resources together:
There was a problem hiding this comment.
what we could do is work on keeping the suffixes the same across modules. Or even make them configurable, example: public IP get's -pip but you can overwrite it with a variable (one variable for all PIP suffixes)
There was a problem hiding this comment.
I think we can be opinionated here with the prefixes(ie. pip, nsg, etc.). I don't have a strong preference. If we need to have a variable to control the suffix let's do that via another PR
FoSix
changed the title
| @@ -3,13 +3,22 @@ resource "azurerm_public_ip" "this" { | |||
|
|
|||
| location = var.location | |||
| resource_group_name = var.resource_group_name | |||
| name = each.value.name | |||
| name = "${each.value.name}-pip" | |||
There was a problem hiding this comment.
I think we can be opinionated here with the prefixes(ie. pip, nsg, etc.). I don't have a strong preference. If we need to have a variable to control the suffix let's do that via another PR
Description
Full refactor of the modules and examples.
BREAKING CHANGES ahead.
Motivation and Context
Some of the modules and examples required a bigger re-work in terms of flexibility and more modern approach to TF code. We're also starting to introduce with this PR configuration options available in recent TF versions (like
nullable).How Has This Been Tested?
All examples were deployed.
Screenshots (if appropriate)
Types of changes
Checklist