feat!: Update autoscaling group `tags` -> `tag` to support v4 of AWS provider #179

bryantbiggs · 2022-02-10T18:57:55Z

Description

Update autoscaling group tags -> tag to support v4 of AWS provider which is now a tag block as opposed to a standard attribute
var.propagate_name has been removed
var.tags_as_map has been moved into var.tags and now var.tags just takes the standard map(string) map of key value pairs for tags. The tags provided are propagated to instances launched through the dynamic tag block
fix example for predictive scaling policy which caught some issues in the nested map lookup logic that has been corrected
var.tags are now automatically merged with var.tag_specifications

Motivation and Context

Avoids showing users a deprecation statement for tags on autoscaling group
Simplify tag usage and removes constant tag updates in each plan/apply

Breaking Changes

Yes
- var.propagate_name has been removed
- var.tags_as_map has been moved into var.tags and now var.tags just takes the standard map(string) map of key value pairs for tags. The tags provided are propagated to instances launched through the dynamic tag block
Users will need to ensure that the value they are passing to var.tags is the standard map of key=value pairs (map(string))

How Has This Been Tested?

I have tested and validated these changes using one or more of the provided examples/* projects

…ovider

antonbabenko · 2022-02-10T19:03:32Z

main.tf

@@ -446,7 +446,14 @@ resource "aws_autoscaling_group" "this" {
    delete = var.delete_timeout
  }

-  tags = local.tags
+  dynamic "tag" {
+    for_each = { for tag in local.tags : "${tag.key}-${tag.value}" => tag }


Suggested change

for_each = { for tag in local.tags : "${tag.key}-${tag.value}" => tag }

for_each = { for tag in local.tags : (tag.key) => tag }

Will this work, too? Value can be changed, so I don't think it is a good idea to use it as a part of a key in a map. WHYT? (Will this even work?)

I couldn't get that to work, it gives a bunch of errors like this

Error: Duplicate object key │ │ on ../../main.tf line 448, in resource "aws_autoscaling_group" "this": │ 448: for_each = { for tag in local.tags : "${tag.key}" => tag } │ ├──────────────── │ │ tag.key is "Project" │ │ Two different items produced the key "Project" in this 'for' expression. If duplicates are expected, use the ellipsis (...) after the value expression to enable grouping by key.

Maybe combine values by key? The key should be unique anyway.

well, in theory yes but not the way that we accept it. we let users pass in multiple tags and they can all have the same key, but the last key in the list is the one used - but the provider handles this deduplication, not us. I'll have to play around with this a bit and see

Right, I wonder what will be the result if a user provides keys in several ways like this: {Project = "foo"} and {Project = "bar"}.

Without deduplication, there will be 2 keys in for_each created - Project-foo and Project-bar. Will it lead to constant changes after the creation?

we could do like this, but we'd have to drop the var.propagate_name variable terraform-aws-modules/terraform-aws-eks@33b4112

we can get away with this in the EKS module because we never offered users the ability to set propagate to true or false, just always defaulted to true

Let's do it to reduce the number of potential issues related to constant changes.

Let me take a deeper look later - not good to rush through this. I have some ideas that should solve both sides of this

bryantbiggs · 2022-02-11T16:11:47Z

examples/complete/main.tf

@@ -557,7 +531,11 @@ module "complete_lt" {
    },
    predictive-scaling = {
      policy_type = "PredictiveScaling"
-      predictive_scaling_config = {


not sure why this wasn't caught before but it was incorrect

Probably nobody was using it :)

bryantbiggs · 2022-02-11T16:13:07Z

main.tf

@@ -542,28 +532,28 @@ resource "aws_autoscaling_policy" "this" {
      scheduling_buffer_time       = lookup(predictive_scaling_configuration.value, "scheduling_buffer_time", null)

      dynamic "metric_specification" {
-        for_each = lookup(predictive_scaling_configuration.value, "metric_specification", [])
+        for_each = can(predictive_scaling_configuration.value.metric_specification.target_value) ? [predictive_scaling_configuration.value.metric_specification] : []


with the change on line 560 of the example now exercising this configuration, I had to make some adjustments to get this to work as intended and without errors

bryantbiggs · 2022-02-11T16:13:36Z

variables.tf

-
-variable "tags_as_map" {
-  description = "A map of tags and values in the same format as other resources accept. This will be converted into the non-standard format that the aws_autoscaling_group requires."
+  description = "A map of tags to assign to resources"


brings everything back inline with other modules where users just provide var.tags

bryantbiggs · 2022-02-11T16:25:48Z

ok should be ready for review now if you get some time @antonbabenko

antonbabenko · 2022-02-11T16:28:18Z

@bryantbiggs I am pretty much off with corona since Wednesday. Only obvious PRs can be processed by me before Monday 🤯

bryantbiggs · 2022-02-11T19:23:54Z

@antonbabenko no rush at all - sorry to hear, hope you feel better soon!

antonbabenko

Looks good. To make this release a "breaking change" I need to include "BREAKING CHANGE" into the commit message (if I remember correctly). Fingers crossed!

antonbabenko · 2022-02-14T10:48:33Z

examples/complete/main.tf

@@ -557,7 +531,11 @@ module "complete_lt" {
    },
    predictive-scaling = {
      policy_type = "PredictiveScaling"
-      predictive_scaling_config = {


Probably nobody was using it :)

## [5.0.0](v4.11.0...v5.0.0) (2022-02-14) ### ⚠ BREAKING CHANGES * Update autoscaling group `tags` -> `tag` to support v4 of AWS provider (#179) ### Features * Update autoscaling group `tags` -> `tag` to support v4 of AWS provider ([#179](#179)) ([2c2a8a9](2c2a8a9))

antonbabenko · 2022-02-14T10:57:48Z

This PR is included in version 5.0.0 🎉

github-actions · 2022-10-28T02:42:02Z

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.

fix: Update autoscaling group tags -> tag to support v4 of AWS pr…

1a5d801

…ovider

bryantbiggs requested a review from antonbabenko February 10, 2022 18:59

antonbabenko reviewed Feb 10, 2022

View reviewed changes

antonbabenko mentioned this pull request Feb 10, 2022

fix: Update autoscaling group tags -> tag to support v4 of AWS provider terraform-aws-modules/terraform-aws-eks#1866

Merged

1 task

chore: clean up propogation

73ccbeb

bryantbiggs marked this pull request as draft February 10, 2022 19:57

bryantbiggs added 2 commits February 11, 2022 09:39

chore: clean up tag usage

6b1e0a1

fix: caught issues in testing

7a9ae41

bryantbiggs commented Feb 11, 2022

View reviewed changes

bryantbiggs changed the title ~~fix: Update autoscaling group tags -> tag to support v4 of AWS provider~~ fix!: Update autoscaling group tags -> tag to support v4 of AWS provider Feb 11, 2022

bryantbiggs marked this pull request as ready for review February 11, 2022 16:25

antonbabenko approved these changes Feb 14, 2022

View reviewed changes

antonbabenko changed the title ~~fix!: Update autoscaling group tags -> tag to support v4 of AWS provider~~ feat!: Update autoscaling group tags -> tag to support v4 of AWS provider Feb 14, 2022

antonbabenko merged commit 2c2a8a9 into terraform-aws-modules:master Feb 14, 2022

bryantbiggs deleted the fix/v4-support branch February 14, 2022 12:08

antonbabenko mentioned this pull request Feb 14, 2022

feat: Remove launch configuration support, add support for ignoring desired_capacity #173

Merged

1 task

github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Update autoscaling group `tags` -> `tag` to support v4 of AWS provider #179

feat!: Update autoscaling group `tags` -> `tag` to support v4 of AWS provider #179

bryantbiggs commented Feb 10, 2022 •

edited

Loading

antonbabenko Feb 10, 2022

bryantbiggs Feb 10, 2022

antonbabenko Feb 10, 2022

bryantbiggs Feb 10, 2022

antonbabenko Feb 10, 2022

bryantbiggs Feb 10, 2022

antonbabenko Feb 10, 2022

bryantbiggs Feb 10, 2022

bryantbiggs Feb 11, 2022

antonbabenko Feb 14, 2022

bryantbiggs Feb 11, 2022 •

edited

Loading

bryantbiggs Feb 11, 2022

bryantbiggs commented Feb 11, 2022

antonbabenko commented Feb 11, 2022

bryantbiggs commented Feb 11, 2022

antonbabenko left a comment

antonbabenko Feb 14, 2022

antonbabenko commented Feb 14, 2022

github-actions bot commented Oct 28, 2022

	for_each = { for tag in local.tags : "${tag.key}-${tag.value}" => tag }
	for_each = { for tag in local.tags : (tag.key) => tag }

feat!: Update autoscaling group tags -> tag to support v4 of AWS provider #179

feat!: Update autoscaling group tags -> tag to support v4 of AWS provider #179

Conversation

bryantbiggs commented Feb 10, 2022 • edited Loading

Description

Motivation and Context

Breaking Changes

How Has This Been Tested?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryantbiggs Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryantbiggs commented Feb 11, 2022

antonbabenko commented Feb 11, 2022

bryantbiggs commented Feb 11, 2022

antonbabenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonbabenko commented Feb 14, 2022

github-actions bot commented Oct 28, 2022

feat!: Update autoscaling group `tags` -> `tag` to support v4 of AWS provider #179

feat!: Update autoscaling group `tags` -> `tag` to support v4 of AWS provider #179

bryantbiggs commented Feb 10, 2022 •

edited

Loading

bryantbiggs Feb 11, 2022 •

edited

Loading