Fix ECS cluster ARN attribute name/value for aws_batch_compute_environment resource

[rxacevedo]

Description

Small change here, the ECS Cluster ARN attribute was named ecc_cluster_arn and was actually being assigned the ARN of the compute environment instead of the cluster. I have updated the code to use the ECS cluster ARN instead, as well updating the attribute name in the code/documentation to ecs_cluster_arn.

Test

AWS_ACCESS_KEY_ID=$(aws configure get aws_access_key_id) AWS_SECRET_ACCESS_KEY=$(aws configure get aws_secret_access_key) AWS_REGION=us-east-1 make testacc TEST=./aws TESTARGS='-run=TestAccAWSBatchComputeEnvironment_createUnmanaged'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSBatchComputeEnvironment_createUnmanaged -timeout 120m
=== RUN   TestAccAWSBatchComputeEnvironment_createUnmanaged
--- PASS: TestAccAWSBatchComputeEnvironment_createUnmanaged (52.43s)
=== RUN   TestAccAWSBatchComputeEnvironment_createUnmanagedWithComputeResources
--- PASS: TestAccAWSBatchComputeEnvironment_createUnmanagedWithComputeResources (41.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	93.764s

[radeksimko]

Hi @rxacevedo
thanks for spotting this and for raising the PR.

In principle the change makes sense, but there may be people who are (for any weird) reason using the wrong field and this PR contains breaking change in that sense.

Would you therefore mind keeping the existing field there and marking it as Deprecated: "Use ecs_cluster_arn or arn"? We can remove it in the next major version.

FYI: We made a promise to stick to semantic versioning of providers from 1.0.0, see https://www.hashicorp.com/blog/hashicorp-terraform-provider-versioning/

Thanks.

[rxacevedo]

Hi @radeksimko - thanks for the prompt response. No problem - I've gone ahead and added ecc_cluster_arn back as an attribute on the Schema and set it to the same value as ecs_cluster_arn and added a Deprecated attribute as I've seen done in other classes. In my local usage of this newly compiled version of the provider, I am not seeing the warnings being printed to the console when I use the Deprecated attribute. Is there extra logic that I need to add in order to have the warning print out, or should adding the attribute be enough? Example:

There are warnings related to your configuration. If no errors occurred,
Terraform will continue despite these warnings. It is a good idea to resolve
these warnings in the near future.

Warnings:

  * aws_appautoscaling_policy.in: "adjustment_type": [DEPRECATED] Use step_scaling_policy_configuration -> adjustment_type instead
  * aws_appautoscaling_policy.in: "cooldown": [DEPRECATED] Use step_scaling_policy_configuration -> cooldown instead
  * aws_appautoscaling_policy.in: "metric_aggregation_type": [DEPRECATED] Use step_scaling_policy_configuration -> metric_aggregation_type instead
  * aws_appautoscaling_policy.in: "step_adjustment": [DEPRECATED] Use step_scaling_policy_configuration -> step_adjustment instead
  * aws_appautoscaling_policy.out: "adjustment_type": [DEPRECATED] Use step_scaling_policy_configuration -> adjustment_type instead
  * aws_appautoscaling_policy.out: "cooldown": [DEPRECATED] Use step_scaling_policy_configuration -> cooldown instead
  * aws_appautoscaling_policy.out: "metric_aggregation_type": [DEPRECATED] Use step_scaling_policy_configuration -> metric_aggregation_type instead
  * aws_appautoscaling_policy.out: "step_adjustment": [DEPRECATED] Use step_scaling_policy_configuration -> step_adjustment instead

I used resource_aws_appautoscaling_policy.go as a reference but am not finding any logic that explicitly prints these warnings, so was wondering if you could point me in the direction. Thanks!

[rxacevedo]

@radeksimko Actually - another observation, this is an exported attribute and not an input on the resource itself, so perhaps there is no warning to print since there is no user input for this? Let me know if allowing both attributes to exist/be exported and having one marked as Deprecated via that attribute is sufficient. Thanks!

[radeksimko]

am not finding any logic that explicitly prints these warnings, so was wondering if you could point me in the direction.

This is actually a known issue I reported a while back hashicorp/terraform#7569

I think we can ignore it from the provider's perspective as there isn't much more we can do here.

Sorry for the confusion.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!