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

Re-adds support for setting tags on the default route table #147

Conversation

lorengordon
Copy link
Contributor

Uses a data source to retrieve and preserve the routes
that are currently associated with the route table. This
works around prior problems with supporting this resource,
as described in #95.

Fixes #146

Uses a data source to retrieve and preserve the routes
that are currently associated with the route table. This
works around prior problems with supporting this resource,
as described in terraform-aws-modules#95.

Fixes terraform-aws-modules#146
@lorengordon
Copy link
Contributor Author

@antonbabenko this seemed pretty straightforward to address, after all.

@antonbabenko
Copy link
Member

Please update README and maybe an example. See #111 for more.

I will try to look at this PR and test it on Monday, but can't promise.

@lorengordon
Copy link
Contributor Author

lorengordon commented Jun 22, 2018

I gated the resource on the manage_default_vpc variable. So as is, the manage-default-vpc example covers the resource. The default DHCP option set will get the same name tag as the default VPC, set with default_vpc_name. Seemed like a reasonable approach that parallels how tags are assigned to the other DHCP option sets.

And I did update the README already, at least to add the variable back to the table. I don't really see the default VPC covered anywhere else there, so I'm not sure what more you're looking for.

@stefansedich
Copy link

stefansedich commented Dec 14, 2019

This would be handy for me, looks like it has been stalled for a loooong time, is there any desire to get it in?

@@ -425,3 +425,23 @@ resource "aws_default_vpc" "this" {

tags = "${merge(map("Name", format("%s", var.default_vpc_name)), var.default_vpc_tags, var.tags)}"
}

data "aws_route_table" "default" {
Copy link

Choose a reason for hiding this comment

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

Why use a data source ?
Route table ID is available by concat(aws_vpc.this.*.default_route_table_id, [""])[0] (see outputs)

count = "${var.manage_default_vpc ? 1 : 0}"

default_route_table_id = "${element(data.aws_route_table.default.*.route_table_id, 0)}"
route = ["${data.aws_route_table.default.*.routes[0]}"]
Copy link

Choose a reason for hiding this comment

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

I made a try outside VPC module, route argument is useless :

Moreover this version is more TF > 0.12 compliant:

resource "aws_default_route_table" "this" {
  count = "${var.manage_default_vpc ? 1 : 0}"

  default_route_table_id = concat(aws_vpc.this.*.default_route_table_id, [""])[0]
  tags = merge({
      Name = var.default_vpc_name
      Createdby = "AWS"
      Taggedby = "terraform"
    },
    var.tags,
    var.default_route_table_tags
  )
}

@gillg
Copy link

gillg commented Apr 19, 2020

This feature seems usefull for lisibility of your infrastructure...
Maybe also, option manage_default_vpc is useless, VPC module should tag all its resources. It's what user wants.

To workaround, for now I made a resource outside vpc module

resource "aws_default_route_table" "default_table" {
  default_route_table_id = module.vpc.default_route_table_id
  tags = {
    Name = "my-vpc-name-default"
    Createdby = "AWS"
    Taggedby = "terraform"
  }
}

That's all, and now all my routes tables are tagged

@DrFaust92
Copy link
Contributor

Hey @lorengordon, can you take a look at the comments left by @gillg?
it's worth to make this PR a more generic "manage the default route table" task. waiting for you response. if you no response is given ill continue this with branch from this.

@lorengordon
Copy link
Contributor Author

@DrFaust92 haven't touched this in two years. feel free to take it over.

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, 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 Oct 31, 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

Successfully merging this pull request may close these issues.

default_route_table_tags not wired up?
6 participants