Add Support for List of Tag Maps #18

Jamie-BitFlight · 2018-05-09T14:53:11Z

what

Add support for list of tag maps

why

Certain kinds of resources (e.g. AWS AutoScaling groups) need them

…isn't required

osterman · 2018-05-09T16:38:20Z

Hah, yes, this is now entirely possible with locals. Question then becomes, what provider does it belong under?

https://www.terraform.io/docs/registry/modules/publish.html#requirements

We might want to create a new module under that provider, since terraform-null-label implies the primary provider is null. I guess it should be in a module called terraform-terraform-label

https://github.com/terraform-providers/terraform-provider-terraform

osterman · 2018-05-09T16:40:01Z

We were recently faced with this conundrum here: cloudposse/terraform-aws-key-pair#12 (see discussion for some context)

Jamie-BitFlight · 2018-05-09T21:28:29Z

I see. So for it to be published publicly it needs to be named by the provider.

The locals is part of core terraform. I imagine that you could name it core or terraform for the middle name?

And retire the null provider repo? Since it requires extra downloading and initializing, unlike locals?

osterman · 2018-05-09T21:48:37Z

Yea, so the provider would be terraform. I would be more than happy to accomodate the repo, but terraform-null-provider will live to see another day.

We have nearly 80 repositories that need to be updated. All-in-all, it's a very low priority since the legacy method works fine.

osterman · 2018-05-09T21:52:52Z

Fwiw, here's a repo if you want to rebase your change ontop of it: https://github.com/cloudposse/terraform-terraform-label

Jamie-BitFlight · 2018-05-10T15:03:40Z

Thank you. I have something even better to show you with my next commit.

Updated to allow for tagging of Autoscaling groups. It was really damn hard guys, really hard to getthat stupid thing expanding as a dynamic list.

Jamie-BitFlight · 2018-05-10T15:32:20Z

@osterman I've created a new technique for use in this module, and I have put the null provider back in to handle it.
Autoscaling groups and their servers can now be tagged dynamically.

Jamie-BitFlight · 2018-05-14T14:34:32Z

@aknysh Is this being reviewed?

…tor section. fixed copy paste error in table.

aknysh · 2018-05-14T16:20:59Z

main.tf

+    }"
+
+  tags_asg_propagate_true  = ["${null_resource.tags_asg_propagate_true.*.triggers}"]
+  tags_asg_propagate_false = ["${null_resource.tags_asg_propagate_false.*.triggers}"]


@Jamie-BitFlight
thanks again!
please remove everything related to other technologies and services from this module.
it's the lowest-level generic module which all other modules use.
it should not mention anything like EC2 or ASG.
(this could be in the advanced examples though)

aknysh · 2018-05-14T16:21:54Z

README.md

@@ -148,6 +229,8 @@ resource "aws_instance" "eg_prod_bastion_xyz" {
 | namespace | Normalized namespace  |
 | stage | Normalized stage  |
 | tags | Merge input tags with our tags. Note: `Name` has a special meaning in AWS and we need to disamgiuate it by using the computed `id`   |
+| tags_asg_propagate_true | a list of maps that contain the ASG format tags with propagation set to true |
+| tags_asg_propagate_false | a list of maps that contain the ASG format tags with propagation set to false |


@Jamie-BitFlight
please remove everything related to other technologies and services from this module.
it's the lowest-level generic module which all other modules use.
it should not mention anything like EC2 or ASG in the inputs.
thanks

aknysh · 2018-05-14T16:22:43Z

main.tf

  }
+}
+
+resource "null_resource" "tags_asg_propagate_false" {


here too :)

aknysh · 2018-05-14T16:23:00Z

outputs.tf


  description = "Normalized Tag map"
 }
+
+output "tags_asg_propagate_true" {


osterman · 2018-05-14T19:22:10Z

I think tags_as_list_of_maps is very descriptive, so I am okay with leaving it as-is.

Just wonder though if we should rename both outputs for consistency:

tags_map - returns map of tags
tags_list - returns list of tag maps

@aknysh thoughts?

Jamie-BitFlight · 2018-05-14T19:25:22Z

If you change tags to tags_map it wont be backwards compatible.

…ments.

osterman · 2018-05-15T18:00:58Z

examples/autoscalinggroup/main.tf

+######################
+# Autoscaling group  #
+######################
+resource "aws_autoscaling_group" "this" {


We typically use default in place of this. Using this is also a good convention, but let's stick with default since that's what we have else where.

osterman · 2018-05-15T18:02:01Z

@aknysh LGTM; one small nitpick on this vs default.

aknysh · 2018-05-16T00:32:38Z

examples/autoscalinggroup/main.tf

+  source    = "../../"
+  namespace = "awesomeproject"
+  stage     = "production"
+  name      = "clusterpluck"


does not sound good.
please use our standard conventions:

namespace = "cp" stage = "prod" name = "app"

aknysh · 2018-05-16T00:33:21Z

README.md

@@ -121,6 +121,93 @@ resource "aws_instance" "eg_prod_bastion_xyz" {
   vpc_security_group_ids = ["${aws_security_group.eg_prod_bastion_xyz.id}"]
 }
 ```
+### Advanced Example 2
+
+Here is a more complex example with an autoscaling group that has a different tagging schema than other resources and requres its tags to be in this format, which this module can generate:


typo in requres

aknysh · 2018-05-16T00:34:24Z

README.md

+  source    = "../../"
+  namespace = "awesomeproject"
+  stage     = "production"
+  name      = "clusterpluck"


does not sound good.
please use our standard conventions:

namespace = "cp" stage = "prod" name = "app"

aknysh · 2018-05-16T00:36:23Z

README.md

+]
+```
+
+Autoscaling group using proagating tagging below (full example: examples/autoscalinggroup/main.tf)


typo in proagating

aknysh · 2018-05-16T00:37:27Z

README.md

+#######################
+# Launch template     #
+#######################
+resource "aws_launch_template" "this" {


change this to default as it's used in all CloudPosse modules

aknysh · 2018-05-16T00:37:56Z

README.md

+######################
+# Autoscaling group  #
+######################
+resource "aws_autoscaling_group" "this" {


change this to default as it's used in all CloudPosse modules

aknysh · 2018-05-16T00:40:59Z

outputs.tf

+
+output "tags_as_list_of_maps" {
+  value       = ["${local.tags_as_list_of_maps}"]
+  description = "Additional tags as a list of maps. Which can be used in several resources."


Additional tags as a list of maps, which can be used in several AWS resources

aknysh · 2018-05-16T00:42:56Z

variables.tf

@@ -32,3 +32,9 @@ variable "tags" {
  default     = {}
  description = "Additional tags (e.g. `map('BusinessUnit`,`XYZ`)"


please also update the quotes
Additional tags (e.g. map('BusinessUnit','XYZ'))

* upstream/master: Another little terraform typo (cloudposse#20)

aknysh

@Jamie-BitFlight
thanks
we are close, just a few comments

* 'master' of github.com:Jamie-BitFlight/terraform-null-label:

Jamie-BitFlight · 2018-05-16T01:15:41Z

Those have been addressed @aknysh

aknysh

+1

aknysh · 2018-05-16T01:42:21Z

@Jamie-BitFlight
thanks a lot for your contribution.
merged to master

Jamie-BitFlight · 2018-05-16T01:44:04Z

Thanks for working with me on this.

Updated to not used null_resource so that the null_resource provider …

1c2ee27

…isn't required

Jamie Nelson added 2 commits May 10, 2018 16:19

Updated to unclude the null provider version.

39b26fb

Updated to allow for tagging of Autoscaling groups. It was really damn hard guys, really hard to getthat stupid thing expanding as a dynamic list.

updated formatting

c175941

Jamie-BitFlight changed the title ~~Updated to not used null_resource so that the null_resource provider …~~ Updated so that the null_resource provider is used for autoscaling tags, and locals for standard tags May 10, 2018

Jamie Nelson added 4 commits May 14, 2018 15:44

Fixed up readme. added better example. Added commends. Added contribu…

0a635cc

…tor section. fixed copy paste error in table.

Clarified and tested the usage of the tags

1e3f27f

Updated the path to the module in the example

0cb08d4

fixed formt

3c6b730

aknysh reviewed May 14, 2018

View reviewed changes

main.tf Outdated

}

}

resource "null_resource" "tags_asg_propagate_false" {

Copy link

Member

aknysh May 14, 2018

Choose a reason for hiding this comment

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

here too :)

aknysh reviewed May 14, 2018

View reviewed changes

outputs.tf Outdated

description = "Normalized Tag map"

}

output "tags_asg_propagate_true" {

Copy link

Member

aknysh May 14, 2018

Choose a reason for hiding this comment

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

and here

Jamie Nelson and others added 3 commits May 14, 2018 18:20

made module generic

9588798

made module generic

d7ced5c

Merge branch 'master' into master

874b3b6

Jamie Nelson added 3 commits May 15, 2018 12:47

Fixed up the autoscaling group example to be very clear with many com…

8890611

…ments.

updated example in README

51e71ce

fixed formatting

13ba4ae

osterman changed the title ~~Updated so that the null_resource provider is used for autoscaling tags, and locals for standard tags~~ Add Support for List of Tag Maps May 15, 2018

osterman assigned aknysh May 15, 2018

osterman added the enhancement New feature or request label May 15, 2018

osterman reviewed May 15, 2018

View reviewed changes

Jamie Nelson and others added 3 commits May 15, 2018 20:29

changed example resource names from 'this' to 'default'

14dcc69

remove provider

d15fb3a

Merge branch 'master' into master

ca2d63e

aknysh reviewed May 16, 2018

View reviewed changes

Jamie Nelson added 2 commits May 16, 2018 01:43

Updated for consistency with other modules, fixed typoes

5b25f97

Merge remote-tracking branch 'upstream/master'

2e6d0d2

* upstream/master: Another little terraform typo (cloudposse#20)

aknysh reviewed May 16, 2018

View reviewed changes

Jamie Nelson added 2 commits May 16, 2018 01:48

Updated variable description

37e2f08

Merge branch 'master' of github.com:Jamie-BitFlight/terraform-null-label

e47f8ab

* 'master' of github.com:Jamie-BitFlight/terraform-null-label:

aknysh approved these changes May 16, 2018

View reviewed changes

aknysh merged commit 70a6ecf into cloudposse:master May 16, 2018

osterman mentioned this pull request Aug 27, 2020

Additional tag map isn't actually merged in with the tags #90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for List of Tag Maps #18

Add Support for List of Tag Maps #18

Jamie-BitFlight commented May 9, 2018 •

edited by osterman

Loading

osterman commented May 9, 2018

osterman commented May 9, 2018

Jamie-BitFlight commented May 9, 2018

osterman commented May 9, 2018 •

edited

Loading

osterman commented May 9, 2018

Jamie-BitFlight commented May 10, 2018

Jamie-BitFlight commented May 10, 2018

Jamie-BitFlight commented May 14, 2018

aknysh May 14, 2018

aknysh May 14, 2018

aknysh May 14, 2018

aknysh May 14, 2018

osterman commented May 14, 2018

Jamie-BitFlight commented May 14, 2018

osterman May 15, 2018

osterman commented May 15, 2018

aknysh May 16, 2018

aknysh May 16, 2018

aknysh May 16, 2018

aknysh May 16, 2018

aknysh May 16, 2018

aknysh May 16, 2018

aknysh May 16, 2018

aknysh May 16, 2018

aknysh left a comment

Jamie-BitFlight commented May 16, 2018

aknysh left a comment

aknysh commented May 16, 2018

Jamie-BitFlight commented May 16, 2018

		@@ -32,3 +32,9 @@ variable "tags" {
		default = {}
		description = "Additional tags (e.g. `map('BusinessUnit`,`XYZ`)"

Add Support for List of Tag Maps #18

Add Support for List of Tag Maps #18

Conversation

Jamie-BitFlight commented May 9, 2018 • edited by osterman Loading

what

why

osterman commented May 9, 2018

osterman commented May 9, 2018

Jamie-BitFlight commented May 9, 2018

osterman commented May 9, 2018 • edited Loading

osterman commented May 9, 2018

Jamie-BitFlight commented May 10, 2018

Jamie-BitFlight commented May 10, 2018

Jamie-BitFlight commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osterman commented May 14, 2018

Jamie-BitFlight commented May 14, 2018

Choose a reason for hiding this comment

osterman commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknysh left a comment

Choose a reason for hiding this comment

Jamie-BitFlight commented May 16, 2018

aknysh left a comment

Choose a reason for hiding this comment

aknysh commented May 16, 2018

Jamie-BitFlight commented May 16, 2018

Jamie-BitFlight commented May 9, 2018 •

edited by osterman

Loading

osterman commented May 9, 2018 •

edited

Loading