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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jan 14, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6218a10
mssql: fix test, use relative expiry for service principal password
manicminer Jan 11, 2021
577e180
mssql: fix persistent diff with azurerm_mssql_virtual_machine.storage…
manicminer Jan 11, 2021
72286c4
mssql: drop capacity arg for result slice
manicminer Jan 12, 2021
8acdb6d
mssql_virtual_machine: always track an empty string for storage_confi…
manicminer Jan 13, 2021
79d87e5
mssql_virtual_machine: set all storage_configuration attributes, but …
manicminer Jan 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 28 additions & 10 deletions azurerm/internal/services/mssql/mssql_virtual_machine_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,15 +486,33 @@ func flattenSqlVirtualMachineStorageConfigurationSettings(input *sqlvirtualmachi
return []interface{}{}
}

return []interface{}{
map[string]interface{}{
"disk_type": string(input.DiskConfigurationType),
"storage_workload_type": storageWorkloadType,
"data_settings": flattenSqlVirtualMachineStorageSettings(input.SQLDataSettings),
"log_settings": flattenSqlVirtualMachineStorageSettings(input.SQLLogSettings),
"temp_db_settings": flattenSqlVirtualMachineStorageSettings(input.SQLTempDbSettings),
},
output := make(map[string]interface{})

if v := string(input.DiskConfigurationType); v != "" {
output["disk_type"] = v
}

if storageWorkloadType != "" {
output["storage_workload_type"] = storageWorkloadType
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if v := flattenSqlVirtualMachineStorageSettings(input.SQLDataSettings); len(v) > 0 {
output["data_settings"] = v
}

if v := flattenSqlVirtualMachineStorageSettings(input.SQLLogSettings); len(v) > 0 {
output["log_settings"] = v
}

if v := flattenSqlVirtualMachineStorageSettings(input.SQLTempDbSettings); len(v) > 0 {
output["temp_db_settings"] = v
}

if len(output) == 0 {
return []interface{}{}
}

return []interface{}{output}
}

func expandSqlVirtualMachineDataStorageSettings(input []interface{}) *sqlvirtualmachine.SQLStorageSettings {
Expand All @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK cool, yeah it works without it too.

for i := range input {
if input[i] != nil {
expandedLuns[i] = int32(input[i].(int))
expandedLuns = append(expandedLuns, int32(input[i].(int)))
}
}

Expand Down
20 changes: 10 additions & 10 deletions azurerm/internal/services/mssql/mssql_virtual_machine_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/hashicorp/go-uuid"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/terraform"

"github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/acceptance"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/acceptance/check"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients"
Expand Down Expand Up @@ -71,7 +70,7 @@ func TestAccMsSqlVirtualMachine_complete(t *testing.T) {
})
}

func TestAccMsSqlVirtualMachine_updateAutoPatching(t *testing.T) {
func TestAccMsSqlVirtualMachine_withAutoPatching(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_mssql_virtual_machine", "test")
r := MsSqlVirtualMachineResource{}

Expand All @@ -93,8 +92,9 @@ func TestAccMsSqlVirtualMachine_updateAutoPatching(t *testing.T) {
})
}

func TestAccMsSqlVirtualMachine_updateKeyVault(t *testing.T) {
func TestAccMsSqlVirtualMachine_withKeyVault(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_mssql_virtual_machine", "test")

r := MsSqlVirtualMachineResource{}
value, err := uuid.GenerateUUID()
if err != nil {
Expand All @@ -106,7 +106,7 @@ func TestAccMsSqlVirtualMachine_updateKeyVault(t *testing.T) {
Config: r.withKeyVault(data, value),
Check: resource.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
check.That(data.ResourceName).Key("r_services_enabled").MatchesRegex(regexp.MustCompile("/*:acctestkv")),
check.That(data.ResourceName).Key("key_vault_credential.0.name").MatchesRegex(regexp.MustCompile("/*:acctestkv")),
),
},
data.ImportStep("key_vault_credential.0.key_vault_url", "key_vault_credential.0.service_principal_name", "key_vault_credential.0.service_principal_secret"),
Expand All @@ -115,20 +115,20 @@ func TestAccMsSqlVirtualMachine_updateKeyVault(t *testing.T) {
Config: r.withKeyVaultUpdated(data, value),
Check: resource.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
check.That(data.ResourceName).Key("r_services_enabled").MatchesRegex(regexp.MustCompile("/*:acctestkv2")),
check.That(data.ResourceName).Key("key_vault_credential.0.name").MatchesRegex(regexp.MustCompile("/*:acctestkv2")),
),
},
data.ImportStep("key_vault_credential.0.key_vault_url", "key_vault_credential.0.service_principal_name", "key_vault_credential.0.service_principal_secret"),
})
}

func TestAccMsSqlVirtualMachine_storageConfigurationSettings(t *testing.T) {
func TestAccMsSqlVirtualMachine_withStorageConfiguration(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_mssql_virtual_machine", "test")
r := MsSqlVirtualMachineResource{}

data.ResourceTest(t, r, []resource.TestStep{
{
Config: r.storageConfigurationSettings(data),
Config: r.withStorageConfiguration(data),
Check: resource.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
Expand Down Expand Up @@ -434,7 +434,7 @@ resource "azuread_service_principal" "test" {
resource "azuread_service_principal_password" "test" {
service_principal_id = azuread_service_principal.test.id
value = "%[3]s"
end_date = "2021-01-01T01:02:03Z"
end_date_relative = "240h"
}

resource "azurerm_mssql_virtual_machine" "test" {
Expand Down Expand Up @@ -514,7 +514,7 @@ resource "azuread_service_principal" "test" {
resource "azuread_service_principal_password" "test" {
service_principal_id = azuread_service_principal.test.id
value = "%[3]s"
end_date = "2021-01-01T01:02:03Z"
end_date_relative = "240h"
}

resource "azurerm_mssql_virtual_machine" "test" {
Expand All @@ -530,7 +530,7 @@ resource "azurerm_mssql_virtual_machine" "test" {
`, r.template(data), data.RandomInteger, value)
}

func (r MsSqlVirtualMachineResource) storageConfigurationSettings(data acceptance.TestData) string {
func (r MsSqlVirtualMachineResource) withStorageConfiguration(data acceptance.TestData) string {
return fmt.Sprintf(`
%[1]s

Expand Down