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

new resource: azurerm_resource_tag #19544

Closed
wants to merge 1 commit into from

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented Dec 3, 2022

Motivation

Rel #11682

With an dedicated general tag resource in combination with ignore_changes = [tags], we are allowed to ignore all tags and define tags thats we want to managed from our side

locals {
  tags = {
    owner = "Terraform"
  }
}

resource "azurerm_resource_group" "example" {
  name     = "example"
  location = "West Europe"

  lifecycle {
    # Ignore tags, since they coming from Azure Policies
    ignore_changes = [tags]
  }
}

# Define additional tags
resource "azurerm_resource_tag" "rg_example" {
  foreach = local.tags

  resource_id = azurerm_resource_group.example.id
  
  key   = each.key
  value = each.value
}

Tests

jkr@joe-nb terraform-provider-azurerm % TF_ACC=1 go test -v ./internal/services/resource -timeout=1000m -run='TestAccResourceTag_*'
=== RUN   TestAccResourceTag_basic
=== PAUSE TestAccResourceTag_basic
=== RUN   TestAccResourceTag_disappears
=== PAUSE TestAccResourceTag_disappears
=== RUN   TestAccResourceTag_withTags
=== PAUSE TestAccResourceTag_withTags
=== CONT  TestAccResourceTag_basic
=== CONT  TestAccResourceTag_withTags
=== CONT  TestAccResourceTag_disappears
--- PASS: TestAccResourceTag_disappears (112.41s)
--- PASS: TestAccResourceTag_basic (118.27s)
--- PASS: TestAccResourceTag_withTags (193.71s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/resource      196.149s

@jkroepke jkroepke force-pushed the tag_resource branch 2 times, most recently from de8f4aa to 2c301f9 Compare December 3, 2022 09:43
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
@katbyte katbyte self-assigned this Dec 5, 2022
@tombuildsstuff
Copy link
Contributor

hey @jkroepke

Thanks for this PR.

Taking a look through here whilst the code for this resource looks reasonable, unfortunately as alluded to in this comment in the request for Default Tags there's quite a number of changes needed for this type of resource to work correctly.

Ultimately there's three problems to take into account for this type of change - the first is that whilst most Azure Resources expose a concept of tags (although some Resource Providers call this metadata or similar), unfortunately different Azure Resources have different implementations of Tags (for example some Resource Providers require all lower case keys/values, some with no special characters, some have a maximum of 15 tags etc).

The second is the way that the Provider is implemented at this point in time, with the majority of resources using a shared Create and Update method, which whilst has its benefits also has a downside insofar as ignore_changes isn't fully usable with this approach. As such the main blocker for #13776 at this point in time is updating the provider to use split Create and Update methods, which allows us to conditionally update properties using d.HasChanges("property_name"), which allows ignore_changes to work correctly. Since the Create and Update methods are combined, they unfortunately don't take these conditional updates into account, which is (ultimately) something that we need to fix.

Finally, some Resource Providers require handling tag updates in a different manner to other Resource Providers - for example AKS, HDInsight, Kusto and Service Fabric (which apply tag updates to only the top-level resources, not the resources they provision) - and others which don't support updates at all.

Ultimately this means that whilst this resource can add/remove a tag, we believe that the majority of users would end up experiencing unexpected issues when using this resource unfortunately.

From a resource design perspective, whilst we've considered a separate resource for azurerm_resource_tags in the past - ultimately we've opted not to ship this both for the reasons outlined above, and that we're planning on supporting Default Tags in the future, which when used in combination with ignore_changes and the Tags support on individual resources provides a means of supporting this without introducing the race conditions outlined above.

Chatting about the azurerm_resource_tag resource internally, whilst we can see the value of this resource, ultimately we believe that the same is true here - as such whilst we'd like to thank you for this contribution, this means that unfortunately we're unable to accept this contribution at this point in time.

However I believe that this scenario will ultimately be enabled by splitting the Create/Update methods to support Conditional Updates (and ignore_changes on individual resources), which is something we're working to enable over time. For the moment I'd suggest subscribing to the Default Tags issue (#13776) where we'll post an update when more of the Create/Update methods have been split.

Thanks!

@jkroepke jkroepke deleted the tag_resource branch December 18, 2022 22:42
@github-actions
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 contributions.
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 Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants