hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.77k stars 9.12k forks source link

[Bug]: aws_ecr_repository data source not fetching tags #38155

Closed si-c613 closed 2 months ago

si-c613 commented 3 months ago

Terraform Core Version

all

AWS Provider Version

5.44.0+

Affected Resource(s)

Expected Behavior

Data source should contain a map of tags

{
          arn                          = "arn:aws:ecr:eu-west-1:1234567890:repository/my-repo"
<truncated>
          tags                         = {
          "my_tag"  = "my_value"
          }
},

Actual Behavior

tags map is null

{
          arn                          = "arn:aws:ecr:eu-west-1:1234567890:repository/my-repo"
<truncated>
          tags                         =  null
},

Relevant Error/Panic Output Snippet

NA

Terraform Configuration Files

output "tags" {
  value =  data.aws_ecr_repository.service.tags
}
data "aws_ecr_repository" "service" {
#  for_each = data.aws_ecr_repositories.this.names
  name = "my-repo"
}

terraform {
  required_version = ">= 1.3.0"

  required_providers {
    aws = {
      source  = "hashicorp/aws"
      version = ">= 5.44.0"
    }
  }
}

provider "aws" {
  region = "eu-west-1"
}

Steps to Reproduce

Create an ECR repo called my-repo with tags Run the above Terraform and observe no tags

Lower provider version to <5.44.0 and observe tags are output

Debug Output

No response

Panic Output

No response

Important Factoids

No response

References

Tagging was removed in #36493 https://github.com/hashicorp/terraform-provider-aws/pull/36493/files#diff-e6db909c3ab1e28e7b307f7f5d244a4c9eb880ae2a93db7b7177e9db22fbc64f There was however no note on the PR as to why

Would you like to implement a fix?

None

github-actions[bot] commented 3 months ago

Community Note

Voting for Prioritization

Volunteering to Work on This Issue

justinretzolk commented 3 months ago

Hey @si-c613 👋 Thank you for taking the time to raise this! Can you verify if you've tested with version 5.54.0 or later? This looks very similar to a bug that was fixed in that version.

si-c613 commented 3 months ago

Hi @justinretzolk 👋 , I just checked both 5.54.0 and 5.56.0 and both return tags as null.

I ran through the code yesterday to validate if it was just a case there was no tags on the image and the section for discovering the tags has been removed and as far as I could see there was no replacement for it at all.

stefanfreitag commented 3 months ago

Hi @si-c613, can confirm what you spotted. My test environment is

❯ terraform version
Terraform v1.9.0
on linux_amd64
+ provider registry.terraform.io/hashicorp/aws v5.56.1

It would be nice to understand why listTags was removed here. I can look into providing a PR for this, but frankly spoken I am still learning Golang. So please bear with me if it takes a bit longer.

acwwat commented 3 months ago

This could be caused by bad/missing generated code due to the @Tags annotation having an extra ). I submitted a PR to fix it.

acwwat commented 3 months ago

Hi @si-c613, can confirm what you spotted. My test environment is

❯ terraform version
Terraform v1.9.0
on linux_amd64
+ provider registry.terraform.io/hashicorp/aws v5.56.1

It would be nice to understand why listTags was removed here. I can look into providing a PR for this, but frankly spoken I am still learning Golang. So please bear with me if it takes a bit longer.

As per Resource Tagging, explicit call to listTags is not necessary since transparent tagging applies in this case:

If the service API does not return the tags directly from reading the resource and requires use of the generated listTags function, do nothing and the transparent tagging mechanism will make the listTags call and save any tags into the Terraform state.

stefanfreitag commented 3 months ago

Hi @acwwat, I actually missed the extra ")" in the annotation and thanks for sharing the link! Is one of the existing acceptance tests for the data source checking for the existence of the tag key/ value pairs? If so, then the missing tags should have been spotted earlier, or?

acwwat commented 3 months ago

Hi @acwwat, I actually missed the extra ")" in the annotation and thanks for sharing the link! Is one of the existing acceptance tests for the data source checking for the existence of the tag key/ value pairs? If so, then the missing tags should have been spotted earlier, or?

Hi @stefanfreitag, the test case TestAccECRRepositoryDataSource_basic would be sufficient to validate since it compares tags between the resource and the data source, so I assumed that the test case was just not run.

Since you asked, I decided to spend a few more minutes to test it only to find that the original tags comparison would always assert to true:

resource.TestCheckResourceAttrPair(resourceName, names.AttrTags, dataSourceName, names.AttrTags),

Looking into the TestCheckResourceAttrPair() function, it seems that some data conversion along the way would have caused the tags attribute to be the same regardless of the content. Comparing the map length instead seems more reliable as the test function has logics for it. Consequently I changed the above call to the following for more accurate test results:

resource.TestCheckResourceAttrPair(resourceName, acctest.CtTagsPercent, dataSourceName, acctest.CtTagsPercent),

Rerunning the tests just now yields good results.

Going forward, once tag testing is migrated to using the new framework, the generated testing should be a lot more robust. I am still learning more about it, so I won't do the migration here :)

github-actions[bot] commented 2 months ago

[!WARNING] This issue has been closed, meaning that any additional comments are hard for our team to see. Please assume that the maintainers will not see them.

Ongoing conversations amongst community members are welcome, however, the issue will be locked after 30 days. Moving conversations to another venue, such as the AWS Provider forum, is recommended. If you have additional concerns, please open a new issue, referencing this one where needed.

github-actions[bot] commented 2 months ago

This functionality has been released in v5.58.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

si-c613 commented 2 months ago

Upgraded to this version and tested. All is well now. Thanks to everyone involved

github-actions[bot] commented 1 month ago

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.