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

Overwrite Name tag? #138

Closed
jlory opened this issue Jun 6, 2018 · 11 comments
Closed

Overwrite Name tag? #138

jlory opened this issue Jun 6, 2018 · 11 comments

Comments

@jlory
Copy link

jlory commented Jun 6, 2018

Looking at: https://github.com/terraform-aws-modules/terraform-aws-vpc/blob/master/main.tf#L21 it seems that you automatically creates a tag called Name with a custom value, is there a way to overwrite that? I need a specific value for the Name tag.

@robh007
Copy link
Contributor

robh007 commented Jun 6, 2018

What do you mean? Specify a different name tag in a specific resource, i.e. override the private subnet name to Application? The name tag is used from the input variable Name.

module "vpc" {
  source = "../../"

  name = "simple-example"

  cidr = "10.0.0.0/16"

  azs             = ["eu-west-1a", "eu-west-1b", "eu-west-1c"]
  private_subnets = ["10.0.1.0/24", "10.0.2.0/24", "10.0.3.0/24"]
  public_subnets  = ["10.0.101.0/24", "10.0.102.0/24", "10.0.103.0/24"]

  enable_nat_gateway = true
  single_nat_gateway = true

  tags = {
    Owner       = "user"
    Environment = "dev"
  }
}  

The way the tags are structured means that the name variable is always the winner. You could add name in the tags map but that would be superseded by the name key as part of the merge function.

@robh007
Copy link
Contributor

robh007 commented Jun 6, 2018

No I'm not aware of anything that would allow you to do that as per my previous comment on how merge works and the structure of the merged tag values. I looked to create a PR to reorder the tags so if required you could override the name on resources but I think you'd lose to much of the dynamic naming on resources. i.e. which AZ something is located in.

@antonbabenko
Copy link
Member

antonbabenko commented Jun 7, 2018

I think if we change the order of tags being merged so that "Name" tag can be overwritten by a user for the resources it will be inconsistent for resources which are AZ aware and those which are not (for eg, public route table is not AZ aware, and private route table is AZ aware).

@jlory
Copy link
Author

jlory commented Jun 7, 2018

My question is more why does the module even try to merge Name? If I have all my tags in one place like this: tags = "${local.common_tags}" locals { common_tags = { CreatedBy = "terraform" Environment = "test" Foo = "test" Name = "test" } }
It won't work because Name is always replaced, so I have to use a separate variable just for that.

@antonbabenko
Copy link
Member

Sorry that it took me so long to reply to this one. I have been thinking about this and I think it will make most sense to implement the following logic:

  1. Set Name tag to var.name
  2. Merge tags with resource-specific tags (eg, var.vpc_tags)
  3. Merge tags with var.tags

So, for aws_vpc it will look like this:

resource "aws_vpc" "this" {
  tags = "${merge(map("Name", format("%s", var.name)), var.vpc_tags, var.tags)}"
}

This way tags will be possible to override on all levels.

What do you think guys? /cc @jlory @robh007

@antonbabenko
Copy link
Member

Check #145 for the code changes

antonbabenko added a commit that referenced this issue Jun 20, 2018
* Allow tags override for all resources (fix for #138)

* Added tags for all resources which were missing them
@antonbabenko
Copy link
Member

v1.36.0 has been released. Now you should be able to override tags properly.

Thanks @jlory for reporting this issue!

@robh007
Copy link
Contributor

robh007 commented Jun 20, 2018

Hi, @antonbabenko The only thing I can think of is that the name override is now a specific value. i.e. DATABASE. I think data lookups on single subnets would then break, because you couldn't filter on unique single resources. #145

I looked to do exactly what you've done a while ago. But I think it removes the dynamic AZ & count elements from the name. I'd personally add an additional tags for customisation.

@robh007
Copy link
Contributor

robh007 commented Jun 20, 2018

I'll add an example.

module "vpc" {
  source = "../terraform-aws-vpc"

  name = "MyVPC"
  cidr = "10.1.0.0/16"

  azs = ["eu-west-1a", "eu-west-1b", "eu-west-1c"]

  private_subnets = ["10.1.0.0/24", "10.1.1.0/24", "10.1.2.0/24"]

  tags = {
    Name = "Application"
  }

}

Creates three subnets, but there all called Application

So this data resource fails because it's expecting 1 item to be returned.

data "aws_subnet" "application" {
tags {
Name = "Application"
}
}

This maybe an old data resource that was replaced with aws_subnet_ids, which works. But I still think you're losing useful information within tags.

I'm wondering if there's a way to still add the ability to override the name but perhaps not loose the dynamic naming using AZ and count.index.

@antonbabenko
Copy link
Member

I think it is a good idea to not put "Name" in AZ-aware tags (eg, redshift_subnet_tags) or tags (which is global) because the value will be overridden.

For the piece of code you wrote I would recommend these changes:

private_subnet_tags = {
  Something-but-not-Name = "something"
}

tags = {
  key = "value"
}

@github-actions
Copy link

github-actions bot commented Nov 5, 2022

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants