aztfmod / terraform-provider-azurecaf

Terraform provider for the Terraform platform engineering for Azure
173 stars 92 forks source link

Allow in-place changes for azurecaf_name #116

Closed rcoy-v closed 2 years ago

rcoy-v commented 3 years ago

Description

Currently, the azurecaf_name resource is recreated for any change. This makes this resource much less appealing, especially when using resource_types, because it will cause downstream resources to be recreated as well. The changes in this pull request removes the need for a recreate; now every change is in-place with the same functionality.

Main changes:

Examples

Current

Say this is already applied:

resource "azurecaf_name" "example" {
  name           = "example"
  resource_types = [
    "azurerm_virtual_network"
  ]
}

resource "azurerm_virtual_network" "example" {
  name  = azurecaf_name.example.results.azurerm_virtual_network
  ...
}

and these additions are made:

resource "azurecaf_name" "example" {
  name           = "example"
  resource_types = [
    "azurerm_virtual_network",
    "azurerm_subnet"
  ]
}

resource "azurerm_virtual_network" "example" {
  name  = azurecaf_name.example.results.azurerm_virtual_network
  ...
}

resource "azurerm_subnet" "example" {
  name                 = azurecaf_name.example.results.azurerm_subnet
  virtual_network_name = azurerm_virtual_network.example.name
  ...
}

The result would be a recreate of both the azurecaf_name and azurerm_virtual_network, and a create of azurerm_subnet.

New

Using the same example above, the azurecaf_name would have an in-place change to results. azurerm_virtual_network would not be changed in any way, since its value in results did not change.azurerm_subnet` would still be a create.

sonarcloud[bot] commented 3 years ago

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

rcoy-v commented 3 years ago

I did not add new tests, as I want to see if these changes would be considered for acceptance first. I can add them before merge.

rcoy-v commented 3 years ago

Doing further testing, I don't think these changes are going to quite do what I was looking for either.

If the azurecaf_name is applied first, then new resources are good to use the new values, and existing resources are not changed.

If azurecaf_name and a new resource consuming a new result are applied at the same time, Terraform will error trying to actually resolve the key reference that does not exist yet.

Error: Missing map element
│
│   on example.tf line 25, in resource "azurerm_resource_group" "example":
│   25:   name     = azurecaf_name.example.results.azurerm_resource_group
│     ├────────────────
│     │ azurecaf_name.example.results is map of string with 2 elements
│
│ This map does not have an element with the key "azurerm_resource_group".

If I try to use the lookup function, that also will produce an error since it tries to resolve immediately.

Error: Provider produced inconsistent final plan
│
│ When expanding the plan for azurerm_resource_group.example
│ to include new values learned so far during apply, provider
│ "registry.terraform.io/hashicorp/azurerm" produced an invalid new value for
│ .name: was cty.StringVal("default"), but now
│ cty.StringVal("rg-example").
│
│ This is a bug in the provider, which should be reported in the provider's
│ own issue tracker.

Leaving this open for thoughts, but at this point I think I'm going to use azurecaf_name with just a singular resource_type and wrap it in a for_each to get the type of reuse I'm looking for.

Nepomuceno commented 3 years ago

This is a little more complex changing the schema requirements name would require you to change the schema version this feature althought it is something that I have implemented in the version 3.0 you can see an early draft of it at https://github.com/aztfmod/terraform-provider-azurecaf/pull/90 you could make a pr to that branch because merging this feature will trigger a major version change we are almost finished with that

arnaudlh commented 2 years ago

closing the merge to master as per @Nepomuceno feedback.