hashicorp / pandora

A suite of single-purpose tools enabling automation for Terraform/Azure
Mozilla Public License 2.0
65 stars 47 forks source link

`tools/generator-terraform`: mapping sdk2schema of models will always set a item to state file #2483

Open wuxu92 opened 1 year ago

wuxu92 commented 1 year ago

in func (m modelToModelAssignmentLine) assignmentForReadMapping will init the sdk filed if it is nil in below code:

https://github.com/hashicorp/pandora/blob/345c443985de0cfdf8fc102023201a502d39964c/tools/generator-terraform/generator/mappings/assignment_model_to_model.go#L75-L83

, and this will always set an item to the model which will encoded to tf state file.

https://github.com/hashicorp/pandora/blob/c316d20771273b211c180c55e6dfb806ac239f26/tools/generator-terraform/generator/mappings/assignment_direct.go#L658-L665

i tried with LoadTestService 2022-12-01 the AccTest raise such error below because the Read method will always set the encryption field even the api return the encryption field as nil.

        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
        -/+ destroy and then create replacement

        Terraform will perform the following actions:

          # azurerm_load_test.test must be replaced
        -/+ resource "azurerm_load_test" "test" {
              ~ data_plane_uri      = "8dfd843b-f118-48a4-8f6f-f68e7b02f708.westeurope.cnt-prod.loadtesting.azure.com" -> (known after apply)
              ~ id                  = "/subscriptions/xxxx/resourceGroups/acctestrg-xxxxxxx/providers/Microsoft.LoadTestService/loadTests/acctest-230508095009469840" -> (known after apply)
                name                = "acctest-230508095009469840"
                # (2 unchanged attributes hidden)

              - encryption { # forces replacement
                  - identity {}
                }
            }

        Plan: 1 to add, 0 to change, 1 to destroy.
--- FAIL: TestAccLoadTest_basic (157.30s)
tombuildsstuff commented 1 year ago

@wuxu92 always setting a value for a field into the state is intentional/by design - whilst terraform-plugin-sdk@v2 (and protocol v5) masks when items aren't set into the state - when we move to terraform-plugin-framework (and protocol v6) all fields will need to be set into the state or an error is surfaced automatically - so requiring this now avoids bugs later (which is why we also require this is provider PR's).

That said, taking a look through above, it appears that the identity block shouldn't be present within the encryption block (and shout be surfaced one level higher) - so I think that case is ultimately a schema issue?

wuxu92 commented 1 year ago

@tombuildsstuff is that always setting a value for a field into the state means for the top-level fields? for these not-top-level fields we should not set them if not responded by API if i understand correctly.

the schema is a little bit different from common, there are two identity blocks in this model, one is in top-level, and other one is under encryption block and this one cause the issue.