azurerm_mssql_virtual_machine: fix tests and persistent diff, use relative expiry for service principal password #10125

manicminer · 2021-01-11T06:39:44Z

No description provided.

jackofallops

Thanks @manicminer - LGTM 👍

manicminer · 2021-01-11T07:33:25Z

The test still doesn't pass, there's a persistent diff affecting a few tests. Fix incoming.

    === CONT  TestAccMsSqlVirtualMachine_withKeyVault
        testing.go:684: Step 0 error: After applying this step, the plan was not empty:
            
            DIFF:
            
            UPDATE: azurerm_mssql_virtual_machine.test
              auto_patching.#:                                 "0" => "0"
              id:                                              "/subscriptions/*******/resourceGroups/acctestRG-mssql-210111071526852118/providers/Microsoft.SqlVirtualMachine/sqlVirtualMachines/acctest-VM-210111071526852118" => "/subscriptions/*******/resourceGroups/acctestRG-mssql-210111071526852118/providers/Microsoft.SqlVirtualMachine/sqlVirtualMachines/acctest-VM-210111071526852118"
              key_vault_credential.#:                          "1" => "1"
              key_vault_credential.0.key_vault_url:            "https://acckv-210111071526852118.vault.azure.net/keys/key-210111071526852118/dd21fd1eba9f42e091936a9fb93494cb" => "https://acckv-210111071526852118.vault.azure.net/keys/key-210111071526852118/dd21fd1eba9f42e091936a9fb93494cb"
              key_vault_credential.0.name:                     "acckv-210111071526852118:acctestkv" => "acckv-210111071526852118:acctestkv"
              key_vault_credential.0.service_principal_name:   "acctestspa210111071526852118" => "acctestspa210111071526852118"
              key_vault_credential.0.service_principal_secret: "d78b538b-c58e-46ac-57d4-247a1afd04a8" => "d78b538b-c58e-46ac-57d4-247a1afd04a8"
              r_services_enabled:                              "false" => "false"
              sql_connectivity_port:                           "1433" => "1433"
              sql_connectivity_type:                           "PRIVATE" => "PRIVATE"
              sql_license_type:                                "PAYG" => "PAYG"
              storage_configuration.#:                         "1" => "0"
              storage_configuration.0.data_settings.#:         "0" => ""
              storage_configuration.0.disk_type:               "" => ""
              storage_configuration.0.log_settings.#:          "0" => ""
              storage_configuration.0.storage_workload_type:   "" => ""
              storage_configuration.0.temp_db_settings.#:      "0" => ""
              virtual_machine_id:                              "/subscriptions/*******/resourceGroups/acctestRG-mssql-210111071526852118/providers/Microsoft.Compute/virtualMachines/acctest-VM-210111071526852118" => "/subscriptions/*******/resourceGroups/acctestRG-mssql-210111071526852118/providers/Microsoft.Compute/virtualMachines/acctest-VM-210111071526852118"

…_configuration block

manicminer · 2021-01-11T23:35:35Z

Tests pass with one unrelated destroy failure:

jackofallops

Thanks @manicminer - Just a couple questions / comments in-line, otherwise LGTM 👍

jackofallops · 2021-01-12T12:28:08Z

azurerm/internal/services/mssql/mssql_virtual_machine_resource.go

@@ -510,10 +528,10 @@ func expandSqlVirtualMachineDataStorageSettings(input []interface{}) *sqlvirtual
 }

 func expandSqlVirtualMachineStorageSettingsLuns(input []interface{}) *[]int32 {
-	expandedLuns := make([]int32, len(input))
+	expandedLuns := make([]int32, 0, len(input))


Not sure we need the capacity arg here since we're ranging over input?

OK cool, yeah it works without it too.

jackofallops · 2021-01-12T12:31:51Z

azurerm/internal/services/mssql/mssql_virtual_machine_resource.go

+	if storageWorkloadType != "" {
+		output["storage_workload_type"] = storageWorkloadType
+	}


this possibly wants setting as an empty string so there's always a value?

Unfortunately the problem is that Terraform shows state drift if the containing list has any elements when there are none in the config. It looks like the API has started returning an array having zero values instead of omitting it from the response, which is why I've made it try and detect that. Is there is a better way to avoid state drift in such circumstances?

agreed we should track an empty string in the state in all cases - but if the nested object is returned as an empty object (e.g. all fields are empty) then we'd need to do conditionally return an empty object rather than the nested object - and presumably raise an API bug for that?

@tombuildsstuff Yep an empty object is exactly what's being returned. I'll raise an API bug today and adjust the logic so that an empty string is saved if the other attributes are present and non-zero.

given this affects the entire block - presuming we're using intermediate variables, we should be able to conditionally check the set and conditionally return the nested object, e.g.:

foo := "" if props.Foo != nil { foo = *props.Foo } bar := "" if props.Bar != nil { bar = *props.Bar } if foo == "" && bar == "" { return []interface{}{} } return []interface{}{ map[string]interface{}{ "foo": foo, "bar": bar, }, }

(we've a few instances of this of late)

Cool, makes sense. Have adjusted it to always set all attributes [in the block], but return an empty slice if they all have zero values.

…guration.0.storage_workload_type if any sibling attributes are present and non-zero

…return empty slice if all have zero values

katbyte

Thanks @manicminer - LGTM 👍

ghost · 2021-01-14T17:38:16Z

This has been released in version 2.43.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.43.0"
}
# ... other configuration ...

ghost · 2021-02-13T14:50:35Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

manicminer added service/mssql Microsoft SQL Server tests labels Jan 11, 2021

manicminer requested a review from a team January 11, 2021 06:39

ghost added the size/XS label Jan 11, 2021

manicminer force-pushed the test/mssql-vm-sp-password branch 2 times, most recently from a23c07e to 8d676fc Compare January 11, 2021 07:11

jackofallops approved these changes Jan 11, 2021

View reviewed changes

ghost added size/S and removed size/XS labels Jan 11, 2021

manicminer changed the title ~~mssql: fix test, use relative expiry for service principal password~~ mssql: fix tests and persistent diff, use relative expiry for service principal password Jan 11, 2021

manicminer added 2 commits January 11, 2021 23:16

mssql: fix test, use relative expiry for service principal password

6218a10

mssql: fix persistent diff with azurerm_mssql_virtual_machine.storage…

577e180

…_configuration block

manicminer force-pushed the test/mssql-vm-sp-password branch from db3f458 to 577e180 Compare January 11, 2021 23:16

manicminer requested a review from jackofallops January 11, 2021 23:35

jackofallops approved these changes Jan 12, 2021

View reviewed changes

manicminer added 3 commits January 12, 2021 16:00

mssql: drop capacity arg for result slice

72286c4

mssql_virtual_machine: always track an empty string for storage_confi…

8acdb6d

…guration.0.storage_workload_type if any sibling attributes are present and non-zero

mssql_virtual_machine: set all storage_configuration attributes, but …

79d87e5

…return empty slice if all have zero values

katbyte changed the title ~~mssql: fix tests and persistent diff, use relative expiry for service principal password~~ azurerm_mssql_virtual_machine: fix tests and persistent diff, use relative expiry for service principal password Jan 14, 2021

katbyte approved these changes Jan 14, 2021

View reviewed changes

katbyte added this to the v2.43.0 milestone Jan 14, 2021

katbyte merged commit ce46f33 into master Jan 14, 2021

katbyte deleted the test/mssql-vm-sp-password branch January 14, 2021 01:09

katbyte added a commit that referenced this pull request Jan 14, 2021

update CHANGELOG.md for #10125

4c44ae7

ghost locked as resolved and limited conversation to collaborators Feb 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_mssql_virtual_machine: fix tests and persistent diff, use relative expiry for service principal password #10125

azurerm_mssql_virtual_machine: fix tests and persistent diff, use relative expiry for service principal password #10125

manicminer commented Jan 11, 2021

jackofallops left a comment

manicminer commented Jan 11, 2021 •

edited

Loading

manicminer commented Jan 11, 2021

jackofallops left a comment

jackofallops Jan 12, 2021

manicminer Jan 12, 2021

jackofallops Jan 12, 2021

manicminer Jan 12, 2021

tombuildsstuff Jan 13, 2021

manicminer Jan 13, 2021

tombuildsstuff Jan 13, 2021

manicminer Jan 13, 2021

katbyte left a comment

ghost commented Jan 14, 2021

ghost commented Feb 13, 2021

azurerm_mssql_virtual_machine: fix tests and persistent diff, use relative expiry for service principal password #10125

azurerm_mssql_virtual_machine: fix tests and persistent diff, use relative expiry for service principal password #10125

Conversation

manicminer commented Jan 11, 2021

jackofallops left a comment

Choose a reason for hiding this comment

manicminer commented Jan 11, 2021 • edited Loading

manicminer commented Jan 11, 2021

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Jan 14, 2021

ghost commented Feb 13, 2021

manicminer commented Jan 11, 2021 •

edited

Loading