terraform-aws-vpc icon indicating copy to clipboard operation
terraform-aws-vpc copied to clipboard

Tags prefix

Open ntampakas opened this issue 3 years ago • 3 comments

This is an enhancement that adds a prefix value to the tags for (almost) all resources. I think it improves the readability, especially when there are more than 1 VPCs (with multiple subnets) in the AWS account. The tags_prefix value is declared inside the vpc module. F.x.

module "vpc" {
  source  = "aws-ia/vpc/aws"
  version = ">= 1.0.0"

  name = local.vpc.name
  tags_prefix              = local.vpc.name
  cidr_block               = "${local.vpc.cidr_prefix}.0.0/16"
  az_count                 = length(data.aws_availability_zones.available.names)
  vpc_enable_dns_hostnames = true
...

ntampakas avatar Jul 29 '22 11:07 ntampakas

Thanks for opening this PR! I see the need your trying to fulfill. question, would either the name_prefix from each subnet type definition or the var.name be the same as the tags_prefix? We could potentially use that instead of a new argument, thoughts?

drewmullen avatar Jul 29 '22 14:07 drewmullen

name_prefix

Thanks for opening this PR! I see the need your trying to fulfill. question, would either the name_prefix from each subnet type definition or the var.name be the same as the tags_prefix? We could potentially use that instead of a new argument, thoughts?

Thank you for your quick response! Indeed, var.name could be used instead of a new arg, but AFAIC name_prefix can receive two different values, so that would be a possible mix up when we would like to use a single constant tag prefix for (all) resources :)

ntampakas avatar Jul 29 '22 16:07 ntampakas

Alright, finally got a chance to look a little deeper again. So the way this works right now is that actually the name_prefix IS used if one is provided, defaulting to the subnet type name:

https://github.com/aws-ia/terraform-aws-vpc/blob/ca35ca9662e0ba87d2972c85980741fe3d58d19d/data.tf#L8

At face value, using var.name as the tag_prefix seems better than creating a new variable and I'd be very open to including that in this module if you want to push up the commit

drewmullen avatar Jul 29 '22 17:07 drewmullen

happy to readdress this if its still an issue

drewmullen avatar Nov 13 '22 19:11 drewmullen