data_source_key_vault, resource_arm_key_vault, resource_arm_automation_account, resource_arm_notification_hub_namespace, resource_arm_relay_namespace: Flatten SKU #3119

WodansSon · 2019-03-25T19:03:25Z

First phase of addressing issue #1500, to flatten the SKU block into a single attribute and unifying the handling of SKU across all resources within the Azure Terraform provider.

Example:

resource "azurerm_automation_account" "test" {
  name     = "automation_account-1"
  sku_name = "Basic"
}

resource "azurerm_key_vault" "test" {
  name     = "key-vault-1"
  sku_name = "premium"
}

resource "azurerm_notification_hub_namespace" "test" {
  name     = "notification-hub-namespace-1"
  sku_name = "Free"
}

resource "azurerm_relay_namespace" "test" {
  name     = "relay-namespace-1"
  sku_name = "Standard"
}

katbyte

Hi @jeffreyCline,

Thank you for the PR, i've left a number of comments inline that need to be addressed before merging.

katbyte · 2019-04-28T22:30:09Z

azurerm/data_source_key_vault.go

@@ -177,7 +177,7 @@ func dataSourceArmKeyVaultRead(d *schema.ResourceData, meta interface{}) error {
 		d.Set("enabled_for_template_deployment", props.EnabledForTemplateDeployment)
 		d.Set("vault_uri", props.VaultURI)

-		if err := d.Set("sku", flattenKeyVaultDataSourceSku(props.Sku)); err != nil {
+		if err := d.Set("sku_name", flattenKeyVaultDataSourceSku(props.Sku)); err != nil {


Is this a mistake? there is no property sku_name in the schema for this resource

No, it is not a mistake... the property coming from Azure will still be props.Sku, so I need to read that and then convert it to the new property sku_name

katbyte · 2019-04-28T22:30:43Z

azurerm/data_source_key_vault.go

-
-	return []interface{}{result}
+func flattenKeyVaultDataSourceSku(sku *keyvault.Sku) string {
+	return string(sku.Name)


Same here,

also we don't need a function to do a simple return string(sku.Name), i would have removed this entire function.

I made it a function just in case there is more that needs to be done in the future, I will go ahead and remove the function.

katbyte · 2019-04-28T22:37:25Z

azurerm/resource_arm_automation_account.go

@@ -221,9 +262,21 @@ func flattenAutomationAccountSku(sku *automation.Sku) []interface{} {
 }

 func expandAutomationAccountSku(d *schema.ResourceData) *automation.Sku {


This is for backward compatibility when both sku and sku_name exist side by side...

katbyte · 2019-04-28T22:38:26Z

azurerm/resource_arm_automation_account.go

@@ -140,6 +158,7 @@ func resourceArmAutomationAccountRead(d *schema.ResourceData, meta interface{})
 	}
 	resGroup := id.ResourceGroup
 	name := id.Path["automationAccounts"]
+	skuName := d.Get("sku_name").(string)


What you can do is mark both as computed until 2.0 and just set both instead of doing tricky logic like this

Would that cause issues when we totally remove sku?

Shouldn't as at that point we'll only be setting the required one.

katbyte · 2019-04-28T22:38:33Z

azurerm/resource_arm_automation_account.go

@@ -169,8 +188,21 @@ func resourceArmAutomationAccountRead(d *schema.ResourceData, meta interface{})
 		d.Set("location", azureRMNormalizeLocation(*location))
 	}

-	if err := d.Set("sku", flattenAutomationAccountSku(resp.Sku)); err != nil {
-		return fmt.Errorf("Error setting `sku`: %+v", err)
+	if skuName == "" {


What you can do is mark both as computed until 2.0 and just set both instead of doing tricky logic like this

Would that cause issues when we totally remove sku?

katbyte · 2019-04-28T22:57:40Z

website/docs/r/notification_hub_authorization_rule.html.markdown

-  sku {
-    name = "Free"
-  }
+  sku = "Free"


Shouldn't this be sku_name?

katbyte · 2019-04-28T22:57:43Z

website/docs/r/notification_hub.html.markdown

-  sku {
-    name = "Free"
-  }
+  sku = "Free"


Shouldn't this be sku_name?

katbyte · 2019-04-28T22:57:46Z

website/docs/r/key_vault_secret.html.markdown

-  sku {
-    name = "premium"
-  }
+  sku = "premium"


Shouldn't this be sku_name?

katbyte · 2019-04-28T22:57:50Z

website/docs/r/key_vault_key.html.markdown

-  sku {
-    name = "premium"
-  }
+  sku = "premium"


Shouldn't this be sku_name?

katbyte · 2019-04-28T22:57:53Z

website/docs/r/key_vault_certificate.html.markdown

-  sku {
-    name = "standard"
-  }
+  sku = "standard"


Shouldn't this be sku_name?

Co-Authored-By: kt <kt@katbyte.me>

katbyte

Thanks for the changes @jeffreyCline,

In addition to the comments i've left inline could you add these properties to the Upgrading to 2.0 guide deprecations section?

Also whilst running the tests i notices they are all failing with:

Test ended in panic.

------- Stdout: -------
=== RUN   TestAccAzureRMKeyVaultKey_basicRSAHSM
=== PAUSE TestAccAzureRMKeyVaultKey_basicRSAHSM
=== CONT  TestAccAzureRMKeyVaultKey_basicRSAHSM

------- Stderr: -------
panic: sku: '': source data must be an array or slice, got string
goroutine 254 [running]:
github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.(*ResourceData).Set(0xc000a57650, 0x3775576, 0x3, 0x322d320, 0x3e02840, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/resource_data.go:193 +0x334
github.com/terraform-providers/terraform-provider-azurerm/azurerm.resourceArmKeyVaultRead(0xc000a57650, 0x36a0e20, 0xc0010ba000, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/azurerm/resource_arm_key_vault.go:372 +0xa38
github.com/terraform-providers/terraform-provider-azurerm/azurerm.resourceArmKeyVaultCreateUpdate(0xc000a57650, 0x36a0e20, 0xc0010ba000, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/azurerm/resource_arm_key_vault.go:323 +0xf2f
github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.(*Resource).Apply(0xc000171700, 0xc000c3c460, 0xc0005e5580, 0x36a0e20, 0xc0010ba000, 0x32dcf01, 0xc001773e98, 0xc001370db0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/resource.go:286 +0x363
github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.(*Provider).Apply(0xc00054b200, 0xc00066fa00, 0xc000c3c460, 0xc0005e5580, 0xc001773f08, 0xc000a1b428, 0x32dfd40)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/provider.go:285 +0x9c
github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/plugin.(*GRPCProviderServer).ApplyResourceChange(0xc0000b3b30, 0x3e3dbe0, 0xc001084780, 0xc0007622a0, 0xc0000b3b30, 0xc0010846f0, 0x3342380)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/plugin/grpc_provider.go:842 +0x87a
github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/internal/tfplugin5._Provider_ApplyResourceChange_Handler(0x3613640, 0xc0000b3b30, 0x3e3dbe0, 0xc001084780, 0xc000c3c0f0, 0x0, 0x0, 0x0, 0xc0009e1200, 0x463)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/internal/tfplugin5/tfplugin5.pb.go:3019 +0x23e
github.com/terraform-providers/terraform-provider-azurerm/vendor/google.golang.org/grpc.(*Server).processUnaryRPC(0xc000e82180, 0x3e5b8e0, 0xc000e82600, 0xc000c9df00, 0xc00109f4d0, 0x6cb14e0, 0x0, 0x0, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/google.golang.org/grpc/server.go:966 +0x4a2
github.com/terraform-providers/terraform-provider-azurerm/vendor/google.golang.org/grpc.(*Server).handleStream(0xc000e82180, 0x3e5b8e0, 0xc000e82600, 0xc000c9df00, 0x0)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/google.golang.org/grpc/server.go:1245 +0xd61
github.com/terraform-providers/terraform-provider-azurerm/vendor/google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc000f89a90, 0xc000e82180, 0x3e5b8e0, 0xc000e82600, 0xc000c9df00)
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/google.golang.org/grpc/server.go:685 +0x9f
created by github.com/terraform-providers/terraform-provider-azurerm/vendor/google.golang.org/grpc.(*Server).serveStreams.func1
	/opt/teamcity-agent/work/458e5e4800bd94f6/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/google.golang.org/grpc/server.go:683 +0xa1

katbyte · 2019-06-24T23:51:58Z

azurerm/data_source_key_vault.go

@@ -26,17 +26,9 @@ func dataSourceArmKeyVault() *schema.Resource {

 			"location": azure.SchemaLocationForDataSource(),

-			"sku": {


Shouldn't we keep this in untill 2.0?

Not in the data source, since it is read only... when it writes to the state file it will persist the new attribute instead of the deprecated attribute to be consistent.

However if someone is relying on this property currently it will break them in weird and wonderful ways with no deprecation notice. We should keep it around until 2.0.

As about we should be setting both sku and sku_name here

katbyte · 2019-06-24T23:52:48Z

azurerm/resource_arm_automation_account.go

@@ -140,6 +158,7 @@ func resourceArmAutomationAccountRead(d *schema.ResourceData, meta interface{})
 	}
 	resGroup := id.ResourceGroup
 	name := id.Path["automationAccounts"]
+	skuName := d.Get("sku_name").(string)


Shouldn't as at that point we'll only be setting the required one.

katbyte · 2019-06-24T23:55:03Z

azurerm/resource_arm_key_vault.go

-							Required: true,
+							Type:             schema.TypeString,
+							Required:         true,
+							DiffSuppressFunc: suppress.CaseDifference,


Why are we changing this to suppress case differences?

This has always been the case for sku... I'm not changing anything as I understand it.

katbyte · 2019-06-24T23:55:23Z

azurerm/resource_arm_key_vault.go

+				ValidateFunc: validation.StringInSlice([]string{
+					string(keyvault.Standard),
+					string(keyvault.Premium),
+				}, true),


As this is a new property could we not ignore case here? All enums should be case sensitive.

yup, yup, that was my bad... I missed that one.

katbyte · 2019-06-24T23:56:11Z

azurerm/resource_arm_automation_account.go

 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						"name": {
 							Type:             schema.TypeString,
-							Optional:         true,
-							Default:          string(automation.Basic),
+							Required:         true,


Why are we changing this to required instead of leaving it as optional? this will break peoples configurations?

katbyte · 2019-06-24T23:57:57Z

azurerm/resource_arm_key_vault_test.go

@@ -306,7 +354,7 @@ func TestAccAzureRMKeyVault_justCert(t *testing.T) {
 			{
 				ResourceName:      resourceName,
 				ImportState:       true,
-				ImportStateVerify: true,
+				ImportStateVerify: false,


katbyte · 2019-06-24T23:58:20Z

azurerm/resource_arm_notification_hub_namespace.go

+							Type:             schema.TypeString,
+							Required:         true,
+							ForceNew:         true,
+							DiffSuppressFunc: suppress.CaseDifference,


Why are we changing the case sensitivity here?

azurerm/resource_arm_notification_hub_namespace_test.go

azurerm/resource_arm_relay_namespace.go

katbyte · 2019-06-25T00:00:08Z

azurerm/resource_arm_relay_namespace_test.go

 				),
 			},
 			{
 				ResourceName:      resourceName,
 				ImportState:       true,
-				ImportStateVerify: true,
+				ImportStateVerify: false,


as above. if they are computed and you are setting both on read they will.

katbyte

Updated inline comments

katbyte · 2019-06-26T18:58:58Z

azurerm/data_source_key_vault.go

@@ -26,17 +26,9 @@ func dataSourceArmKeyVault() *schema.Resource {

 			"location": azure.SchemaLocationForDataSource(),

-			"sku": {


However if someone is relying on this property currently it will break them in weird and wonderful ways with no deprecation notice. We should keep it around until 2.0.

azurerm/resource_arm_notification_hub_namespace.go

WodansSon · 2019-06-27T21:50:51Z

katbyte

Thanks for the changes @jeffreyCline,

Just a couple minor comments left to address

azurerm/resource_arm_notification_hub_namespace.go

azurerm/resource_arm_relay_namespace.go

katbyte · 2019-06-27T22:13:14Z

azurerm/data_source_key_vault.go

@@ -26,17 +26,9 @@ func dataSourceArmKeyVault() *schema.Resource {

 			"location": azure.SchemaLocationForDataSource(),

-			"sku": {


As about we should be setting both sku and sku_name here

Co-Authored-By: kt <kt@katbyte.me>

katbyte

Thanks for the revisions @jeffreyCline, aside from the last 2 comments this LGTM

katbyte · 2019-06-27T22:36:29Z

azurerm/data_source_key_vault.go

 		if err := d.Set("sku", flattenKeyVaultDataSourceSku(props.Sku)); err != nil {
 			return fmt.Errorf("Error setting `sku` for KeyVault %q: %+v", *resp.Name, err)
 		}

+		if err := d.Set("sku_name", string(props.Sku.Name)); err != nil {


We'll need to check that props.Sku is not nil here:

if sku := props.Sku; sku != nil { d.Set("sku_name", sku.Name)) }

azurerm/resource_arm_notification_hub_namespace_test.go

Co-Authored-By: kt <kt@katbyte.me>

ghost · 2019-07-28T14:51:07Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

WodansSon added 4 commits March 22, 2019 14:52

Flatten azurerm_automation_account sku

ed02bc9

Flatten azurerm_key_vault sku

4376934

Flatten azurerm_notification_hub sku

857900f

Flatten azurerm_relay_namespace sku

f0f0f7d

WodansSon added service/key-vault Key Vault service/automation service/relay service/notifications refactor labels Mar 25, 2019

WodansSon added this to the v2.0.0 milestone Mar 25, 2019

WodansSon requested review from tombuildsstuff, katbyte, metacpp, mbfrahry and JunyiYi March 25, 2019 19:03

ghost added size/XL documentation labels Mar 25, 2019

WodansSon added 4 commits March 25, 2019 14:44

Fix lint error

8e73fe9

Update for backward compatibility

d630381

Updates for backwards compatibility

a68fd36

Merge branch 'master' into e_flatten_sku

96cf7e3

katbyte requested changes Apr 28, 2019

View reviewed changes

katbyte added the waiting-response label May 7, 2019

katbyte force-pushed the master branch from 77d8dd0 to fa53685 Compare May 12, 2019 19:43

WodansSon and others added 6 commits June 19, 2019 18:25

Update website/docs/r/automation_account.html.markdown

106e746

Co-Authored-By: kt <kt@katbyte.me>

Update website/docs/r/key_vault.html.markdown

2764db9

Co-Authored-By: kt <kt@katbyte.me>

Update website/docs/r/notification_hub_namespace.html.markdown

d15ee4c

Co-Authored-By: kt <kt@katbyte.me>

Update website/docs/r/relay_namespace.html.markdown

907f35e

Co-Authored-By: kt <kt@katbyte.me>

Addressed PR comments...

2bd0c80

Merge branch 'master' into e_flatten_sku

699cd49

WodansSon added 2 commits June 20, 2019 18:58

Fix CI\CD issues

f907f33

Ran gofmt on files...

9781279

katbyte requested changes Jun 25, 2019

View reviewed changes

katbyte added the waiting-response label Jun 25, 2019

WodansSon added 3 commits June 25, 2019 18:09

Updates per PR

c642cdc

Update for PR

465a3ee

Fix lint error

c7cb43b

katbyte requested changes Jun 26, 2019

View reviewed changes

WodansSon added 3 commits June 27, 2019 12:48

Updated CreateUpdate for all resources

a406b85

Fixed relay.SkuName issue

4bd21fa

Fixed Travis Errors

7ac5cef

WodansSon removed the waiting-response label Jun 27, 2019

katbyte changed the title ~~Flatten SKU attribute~~ azurerm_postgresql_server, resource_arm_express_route_circuit, resource_arm_cognitive_account, azurerm_app_service_plan: Flatten SKU Jun 27, 2019

katbyte modified the milestones: v2.0.0, v1.31.0 Jun 27, 2019

katbyte requested changes Jun 27, 2019

View reviewed changes

WodansSon and others added 3 commits June 27, 2019 15:18

Update azurerm/resource_arm_relay_namespace.go

0c8c18b

Co-Authored-By: kt <kt@katbyte.me>

Update azurerm/resource_arm_notification_hub_namespace.go

962aec1

Co-Authored-By: kt <kt@katbyte.me>

Set both Sku and Sku_Name in datasource

d5e7a70

katbyte approved these changes Jun 27, 2019

View reviewed changes

WodansSon and others added 3 commits June 27, 2019 15:46

Update azurerm/resource_arm_notification_hub_namespace_test.go

c6ebd0f

Co-Authored-By: kt <kt@katbyte.me>

Update Key Vault Datasource and test case

86bebe7

Merge branch 'master' into e_flatten_sku

832d630

katbyte merged commit 5686e47 into master Jun 27, 2019

katbyte deleted the e_flatten_sku branch June 27, 2019 23:59

katbyte added a commit that referenced this pull request Jun 28, 2019

Update CHANGELOG.md to include #3119

03966b0

ghost locked and limited conversation to collaborators Jul 28, 2019

		@@ -221,9 +262,21 @@ func flattenAutomationAccountSku(sku *automation.Sku) []interface{} {
		}

		func expandAutomationAccountSku(d schema.ResourceData) automation.Sku {

		@@ -26,17 +26,9 @@ func dataSourceArmKeyVault() *schema.Resource {

		"location": azure.SchemaLocationForDataSource(),

		"sku": {

data_source_key_vault, resource_arm_key_vault, resource_arm_automation_account, resource_arm_notification_hub_namespace, resource_arm_relay_namespace: Flatten SKU #3119

data_source_key_vault, resource_arm_key_vault, resource_arm_automation_account, resource_arm_notification_hub_namespace, resource_arm_relay_namespace: Flatten SKU #3119

Conversation

WodansSon commented Mar 25, 2019 • edited by katbyte Loading

katbyte 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

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

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

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

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

Choose a reason for hiding this comment

WodansSon commented Jun 27, 2019

katbyte left a 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

Choose a reason for hiding this comment

ghost commented Jul 28, 2019

WodansSon commented Mar 25, 2019 •

edited by katbyte

Loading