Skip to content

Conversation

@esacteksab
Copy link
Contributor

When using this module with AWS Provider 6.3.0, I encounter an error

│ Warning: Deprecated attribute
│
│   on .terraform/modules/app_ecs_service/main.tf line 58, in locals:58:             "awslogs-region"        = data.aws_region.current.name
│
│ The attribute "name" is deprecated. Refer to the provider documentation for details.
│
│ (and 2 more similar warnings elsewhere)

This was deprecated with 6.0.0.

I will cut a new major version v8.0.0.

options = {
"awslogs-group" = local.awslogs_group
"awslogs-region" = data.aws_region.current.name
"awslogs-region" = data.aws_region.current.region
Copy link
Contributor Author

Choose a reason for hiding this comment

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

aws = {
source = "hashicorp/aws"
version = ">= 4.6.0"
version = "~> 6.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the deprecation, I'm setting 6.0+ here.

Copy link

@melissathai melissathai left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but let's see what @clint-truss thinks since OHS relies on this module.

Copy link
Contributor

@clint-truss clint-truss left a comment

Choose a reason for hiding this comment

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

Had one question. I haven't finished the review - had to step into a meeting. Did any other variables change their defaults? It's hard to tell with the re-order.

description = "Application Load Balancer (ALB) security group ID to allow traffic from."
default = ""
type = string
default = "DISABLED"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious as to the rationale for this change. The default setting for this attribute is "DISABLED" per the TF docs. Why set it to ""?
Docs: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ecs_service#availability_zone_rebalancing-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's the way the diff is presenting the tfsort But you set this value in this commit and it's reflected here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this view helps support this.

@esacteksab
Copy link
Contributor Author

Had one question. I haven't finished the review - had to step into a meeting. Did any other variables change their defaults? It's hard to tell with the re-order.

I changed no values. I simply did a tfsort.

@esacteksab esacteksab requested a review from clint-truss July 14, 2025 20:11
@clint-truss
Copy link
Contributor

Had one question. I haven't finished the review - had to step into a meeting. Did any other variables change their defaults? It's hard to tell with the re-order.

I changed no values. I simply did a tfsort.

Oh ok, then it just looked that way in the diff. Thanks for the update!

@esacteksab esacteksab merged commit 873a48f into main Jul 14, 2025
1 check passed
@esacteksab esacteksab deleted the feat/aws-6-0 branch July 14, 2025 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants