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

fix: Add missing tag variable in autoscaling target resource #135

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

lays147
Copy link
Contributor

@lays147 lays147 commented Nov 16, 2023

Description

Add the tagsvariable into the aws_appautoscaling_target.

Motivation and Context

I'm using this module to setup an ECS Service and our checkov complained about missing tags in the aws_appautoscaling_target. This PR has the goal to add the tags to this resource.

Breaking Changes

None.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

Signed-off-by: Lays Rodrigues <laysrodriguessilva@gmail.com>
@bryantbiggs bryantbiggs changed the title fix: add missing tag variable in autoscaling target resource fix: Add missing tag variable in autoscaling target resource Nov 16, 2023
Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

thank you!

@bryantbiggs bryantbiggs merged commit 5872445 into terraform-aws-modules:master Nov 16, 2023
12 of 13 checks passed
antonbabenko pushed a commit that referenced this pull request Nov 16, 2023
### [5.7.1](v5.7.0...v5.7.1) (2023-11-16)

### Bug Fixes

* Add missing tag variable in autoscaling target resource ([#135](#135)) ([5872445](5872445))
@antonbabenko
Copy link
Member

This PR is included in version 5.7.1 🎉

@darrell-switch
Copy link

This seems to have broken something, haven't looked into it just yet

│   on .terraform/modules/dev.ecs_cluster_dummy/modules/service/main.tf 
| line 1205, in resource "aws_appautoscaling_target" "this":
│ 1205:   tags               = var.tags

│ An argument named "tags" is not expected here.

@lays147
Copy link
Contributor Author

lays147 commented Nov 17, 2023

@darrell-switch that's odd. Because I have used this module with this patch, and my terraform is not broken. Maybe is the version of terraform? Or the provider?

I'm using terraform 1.6 and aws ~> 5.0 provider.

@darrell-switch
Copy link

Yep, had an old version pinned here 👍
All good 👌

@jasondamour
Copy link
Contributor

Since this forces a provider update (aws v4 is no longer supported with this update), it should maybe be released as a major version. Our version regex picked up this change automatically and it broke our terraform

@bryantbiggs
Copy link
Member

its still a v4.x provider change so no module major version required, but the MSV should have been bumped - #141

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 Dec 28, 2023
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.

None yet

5 participants