Azure / terraform-azurerm-avm-res-network-virtualnetwork

Azure Verified Module for Virtual Network
https://registry.terraform.io/modules/Azure/avm-res-network-virtualnetwork
MIT License
18 stars 14 forks source link

Fixed subnet creation and other fixes #20

Closed herms14 closed 7 months ago

herms14 commented 8 months ago
kewalaka commented 8 months ago

nice work, this goes a long way to fix #17 from my perspective.

The outputs should look something like this (noting the reference to the AVM spec).

# Module owners should include the full resource via a 'resource' output
# https://azure.github.io/Azure-Verified-Modules/specs/terraform/#id-tffr2---category-outputs---additional-terraform-outputs
output "resource" {
  value       = azurerm_virtual_network.vnet
  description = "This is the full resource output for the virtual network resource."
}

Since I assume we want to encourage use of output through the 'resource' full output, I would recommend removing the others.

kewalaka commented 8 months ago

I would suggest moving the subnet code to "main.subnet.tf" - similar to the child resource pattern used by Matt White in the KeyVault module. More crucially, the child resources also need to support the shared AVM interfaces, i.e. you need to be able to set role assignments on subnets too (I don't think subnets support locks).

Ref: https://azure.github.io/Azure-Verified-Modules/specs/shared/#id-snfr24---category-testing---testing-child-extension--interface-resources

kewalaka commented 8 months ago

The AVM template has an updated terraform.tf - you need at least 1.3.0 to support the optional keyword, from the template:

terraform {
  required_version = ">= 1.3.0"
  required_providers {
    # TODO: Ensure all required providers are listed here.
    azurerm = {
      source  = "hashicorp/azurerm"
      version = ">= 3.71.0"
    }
    random = {
      source  = "hashicorp/random"
      version = ">= 3.5.0"
    }
  }
}
kewalaka commented 8 months ago

You are not allowed to delete the mandatory files in this repo.

Total execution time: 1.23 seconds

To address this I would suggest updating the ./.github/actions, policies & workflows folder from the template to make sure this resource is aligned to the required checks - I can spot several differences.

For background - the docs-check had a bug that was fixed here, which might have been blocking you previously https://github.com/Azure/terraform-azurerm-avm-template/issues/37

kewalaka commented 8 months ago

the AVM template now ignores terraform lock files, on line 29, add:

# Ignore Terraform lock file
.terraform.lock.hcl
kewalaka commented 8 months ago

in the locals.tf - I would personally use a single locals block, and you can remove the "TODO" & commented out code at the top of the file.

microsoft-github-policy-service[bot] commented 7 months ago

You are not allowed to delete the mandatory files in this repo.

Total execution time: 1.37 seconds