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

Remove all patches related to tagsAll #4219

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

corymhall
Copy link
Contributor

@corymhall corymhall commented Jul 12, 2024

This PR removes all patches related to the tagsAll implementation and restores the upstream implementation. The new (newer than our hack) upstream tagging implementation utilizes a combination of storing defaultTags in context and utilizing a provider interceptor

In the current implementation there is a breaking change. Previously tags always equaled tagsAll so you could reference the tags output value to get all the tags on the resource. Now these are two separate fields and you would need to reference the tagsAll output to get all the tags. I'm not sure how many people will reference the tags output, but if we wanted to avoid this breaking change until the next major version we could utilize the TransformOutputs callback to always set tags=tagsAll.

Remaining Issues

This PR removes all patches related to the `tagsAll` implementation and
restores the upstream implementation. The new (newer than our hack) upstream tagging
implementation utilizes a combination of storing `defaultTags` in
context and utilizing a [provider interceptor](https://github.com/hashicorp/terraform-provider-aws/blob/3b6772b0864505f9232f737e3d72d80f14043609/internal/provider/intercept.go)

- Updates the s3legacy tagging implementation to match the s3
  implementation (via patch)
- Updates the `applyTags` `PreCheckCallback` to only remove "__defaults"
  items from tags (see #4204)

In the current implementation there is a breaking change. Previously
`tags` always equaled `tagsAll` so you could reference the `tags` output
value to get all the tags on the resource. Now these are two separate
fields and you would need to reference the `tagsAll` output to get all
the tags. I'm not sure how many people will reference the `tags` output,
but if we wanted to avoid this breaking change until the next major
version we could utilize the `TransformOutputs` callback to always set
`tags=tagsAll`.
@corymhall corymhall marked this pull request as draft July 12, 2024 14:54
@mjeffryes mjeffryes modified the milestone: 0.107 Jul 24, 2024
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.

2 participants