azurerm_api_management_api - deprecate `sku` in favour of the `sku_name` property #3154

WodansSon · 2019-04-01T20:57:15Z

Continuation of addressing issue #1500, 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_api_management_api" "test" {
  name      = "api-management-1"
  sku_name  = "Developer_1"
}

r0bnet · 2019-04-02T14:06:01Z

Thanks for this. Without this feature it's a nightmare to create something like t-shirt sizes if you need to define 3 values for each size (for instance for Postgres). Hope this will find it's way into 2.0.0

katbyte

Hi @jeffreyCline,

Thanks for the PR, I've left some comments inline most of which are in the same vane as the other #3119 PR.

azurerm/data_source_api_management.go

katbyte · 2019-04-28T23:04:12Z

azurerm/resource_arm_api_management.go

 						"capacity": {
 							Type:         schema.TypeInt,
 							Required:     true,
-							ValidateFunc: validation.IntAtLeast(0),
+							ValidateFunc: validation.IntAtLeast(1),


How come this value changed?

Per the SDK implementation:

// Capacity - Capacity of the SKU (number of deployed units of the SKU). The default value is 1.

that says default not minimum? thou it does make sense

Oh didn't mean it needed to have a default added, I mean it's all moot as this is removed in a couple version so 🤷‍♀

was more this would break anyone using 0 currently

Reverted code and made it like it was, to avoid potential impact to existing users...

katbyte · 2019-04-28T23:04:43Z

azurerm/resource_arm_api_management.go

+				Type:          schema.TypeString,
+				Optional:      true,
+				ConflictsWith: []string{"sku"},
+				ValidateFunc: azure.SkuNameStringInSlice([]string{


This function seems rather specific to this service?

There are lots of SKU's that have the format of Name_Capacity so I will be able to reuse this function in other resources flatten SKU PR's. I have updated the name so it is more descriptive of what it is actually doing.

katbyte · 2019-04-28T23:04:56Z

azurerm/resource_arm_api_management.go

@@ -366,6 +382,14 @@ func resourceArmApiManagementServiceCreateUpdate(d *schema.ResourceData, meta in
 	client := meta.(*ArmClient).apiManagementServiceClient
 	ctx := meta.(*ArmClient).StopContext

+	log.Printf("[INFO] validating SKU for API Management Service.")


this seems like a mistake?

Removed the line of code...

katbyte · 2019-04-28T23:05:33Z

azurerm/resource_arm_api_management.go

@@ -562,8 +585,21 @@ func resourceArmApiManagementServiceRead(d *schema.ResourceData, meta interface{
 		}
 	}

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


As mentioned in other PR, we can just set both and simplify this logic

I now set both and removed the logic...

katbyte · 2019-04-28T23:07:52Z

azurerm/resource_arm_api_management_api_test.go

@@ -113,6 +154,10 @@ func TestAccAzureRMApiManagementApi_soapPassthrough(t *testing.T) {
 				ResourceName:      resourceName,
 				ImportState:       true,
 				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{


We shouldn't be doing this.

katbyte · 2019-04-28T23:07:58Z

azurerm/resource_arm_api_management_api_test.go

@@ -139,6 +184,8 @@ func TestAccAzureRMApiManagementApi_importSwagger(t *testing.T) {
 				ImportState:       true,
 				ImportStateVerify: true,
 				ImportStateVerifyIgnore: []string{
+					"sku",


We shouldn't be doing this.

katbyte · 2019-04-28T23:08:05Z

azurerm/resource_arm_api_management_api_test.go

@@ -168,6 +215,8 @@ func TestAccAzureRMApiManagementApi_importWsdl(t *testing.T) {
 				ImportState:       true,
 				ImportStateVerify: true,
 				ImportStateVerifyIgnore: []string{
+					"sku",


katbyte · 2019-04-28T23:08:21Z

azurerm/resource_arm_api_management_test.go

@@ -30,6 +31,58 @@ func TestAccAzureRMApiManagement_basic(t *testing.T) {
 				ResourceName:      resourceName,
 				ImportState:       true,
 				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{


We shouldn't be doing this.

katbyte · 2019-04-28T23:08:45Z

website/docs/r/api_management.html.markdown

@@ -46,7 +43,9 @@ The following arguments are supported:

 * `publisher_email` - (Required) The email of publisher/company.

-* `sku` - (Required) A `sku` block as documented below.
+* `sku`  - (Required) A `sku` block as documented below


Can we mark this as deprecated?

…form-provider-azurerm into r_flatten_api_management

katbyte

Thanks for the revisions @jeffreyCline, Overall looking pretty good and i've just left a couple comments inline.

azurerm/data_source_api_management.go

katbyte · 2019-09-23T02:28:28Z

azurerm/resource_arm_api_management.go

 						"capacity": {
 							Type:         schema.TypeInt,
 							Required:     true,
-							ValidateFunc: validation.IntAtLeast(0),
+							ValidateFunc: validation.IntAtLeast(1),


that says default not minimum? thou it does make sense

WodansSon · 2019-09-24T00:32:02Z

Added SKU back into the DataSource...

katbyte

Thanks for the revisions, aside from a couple comments LGTM 👍

katbyte · 2019-09-24T00:54:00Z

azurerm/data_source_api_management.go

-							Computed: true,
-						},
-					},
+				Elem: &schema.Resource{ 


I think you have some trailing whitespace here

katbyte · 2019-09-24T00:54:29Z

azurerm/data_source_api_management.go

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


minor:

Suggested change

// Remove in 2.0

// todo Remove in 2.0

katbyte · 2019-09-24T00:54:40Z

azurerm/data_source_api_management.go

@@ -40,23 +40,29 @@ func dataSourceApiManagementService() *schema.Resource {
 				Computed: true,
 			},

+			// Remove in 2.0


minor:

Suggested change

// Remove in 2.0

// todo Remove in 2.0

katbyte · 2019-09-24T00:57:58Z

azurerm/resource_arm_api_management.go

 						"capacity": {
 							Type:         schema.TypeInt,
 							Required:     true,
-							ValidateFunc: validation.IntAtLeast(0),
+							ValidateFunc: validation.IntAtLeast(1),


Oh didn't mean it needed to have a default added, I mean it's all moot as this is removed in a couple version so 🤷‍♀

katbyte · 2019-09-24T00:58:10Z

azurerm/resource_arm_api_management.go

 						"capacity": {
 							Type:         schema.TypeInt,
 							Required:     true,
-							ValidateFunc: validation.IntAtLeast(0),
+							ValidateFunc: validation.IntAtLeast(1),


was more this would break anyone using 0 currently

katbyte · 2019-09-24T00:58:58Z

azurerm/resource_arm_api_management_test.go

@@ -36,6 +37,50 @@ func TestAccAzureRMApiManagement_basic(t *testing.T) {
 	})
 }

+// Remove in 2.0


Suggested change

// Remove in 2.0

// todo Remove in 2.0

katbyte · 2019-09-24T00:59:05Z

azurerm/resource_arm_api_management_test.go

+	})
+}
+
+// Remove in 2.0


Suggested change

// Remove in 2.0

// todo Remove in 2.0

katbyte · 2019-09-24T01:00:19Z

website/docs/r/api_management.html.markdown

-* `sku` - (Required) A `sku` block as documented below.
+* `sku`  - (Deprecated) A `sku` block as documented below
+
+* `sku_name` - (Required) `sku_name` is a string consisting of two parts separated by an underscore(_). The fist part is the `name`, valid values include: `Developer`, `Basic`, `Standard` and `Premium`. The second part is the `capacity` (e.g. the number of deployed units of the `sku`), which must be a positive `integer` (e.g. `Developer_1`).


You'll need to escape the _ as its all in italics now? or is it just the guthub viewer here

katbyte · 2019-09-24T01:00:47Z

website/docs/r/api_management.html.markdown


 * `capacity` - (Required) Specifies the Pricing Capacity for the API Management Service.

+-> **Note:** This property has been deprecated in favour of the `sku_name` property and will be removed in version 2.0 of the provider.


Could we add this to the upgrading to 2.0 guide found in the guides directory?

Sure... added both the data source and the resource.

ghost · 2019-10-04T10:42:21Z

This has been released in version 1.35.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 = "~> 1.35.0"
}
# ... other configuration ...

ghost · 2020-03-29T15:19:54Z

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!

Flatten arm_api_management sku

f8e65b3

WodansSon added enhancement service/api-management refactor labels Apr 1, 2019

WodansSon added this to the v2.0.0 milestone Apr 1, 2019

WodansSon requested review from tombuildsstuff, katbyte, metacpp, mbfrahry and JunyiYi April 1, 2019 20:57

ghost added size/XL documentation labels Apr 1, 2019

WodansSon added 4 commits April 3, 2019 14:26

Merge branch 'master' into r_flatten_api_management

c751212

Updates for backwards compatibility

770e9a8

Pull from Master

e476239

Updated test cases

ac431ec

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 added 6 commits June 28, 2019 14:11

Pull Master

cc231e9

Merge branch 'master' of https://github.com/terraform-providers/terra…

8318230

…form-provider-azurerm into r_flatten_api_management

Addressing some PR comments

27d7a49

Fix for test pass

e03bbdf

Rmoved comments and fixed docs

e740910

Fixed ImportStateVerify

bf71d8d

WodansSon removed the waiting-response label Sep 19, 2019

WodansSon modified the milestones: v2.0.0, v1.35.0 Sep 19, 2019

katbyte requested changes Sep 23, 2019

View reviewed changes

katbyte added the waiting-response label Sep 23, 2019

Added SKU back into datasource

615d3d8

WodansSon removed the waiting-response label Sep 24, 2019

katbyte approved these changes Sep 24, 2019

View reviewed changes

katbyte changed the title ~~Flatten arm_api_management sku~~ azurerm_api_management_api - deprecate sku in favour of the sku_name property Sep 24, 2019

WodansSon added 3 commits September 24, 2019 12:51

Updated code per PR comments

a371766

Removed newline

9889fad

Remove unused function

0d79609

WodansSon merged commit a1d0ce1 into master Sep 24, 2019

WodansSon deleted the r_flatten_api_management branch September 24, 2019 23:02

WodansSon added a commit that referenced this pull request Sep 24, 2019

added update for #3154 to address issue #1500

8857b75

ghost locked and limited conversation to collaborators Mar 29, 2020


		* `capacity` - (Required) Specifies the Pricing Capacity for the API Management Service.

		-> Note: This property has been deprecated in favour of the `sku_name` property and will be removed in version 2.0 of the provider.

azurerm_api_management_api - deprecate sku in favour of the sku_name property #3154

azurerm_api_management_api - deprecate sku in favour of the sku_name property #3154

Conversation

WodansSon commented Apr 1, 2019 • edited Loading

r0bnet commented Apr 2, 2019

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

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 Sep 24, 2019

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

ghost commented Oct 4, 2019

ghost commented Mar 29, 2020

azurerm_api_management_api - deprecate `sku` in favour of the `sku_name` property #3154

azurerm_api_management_api - deprecate `sku` in favour of the `sku_name` property #3154

WodansSon commented Apr 1, 2019 •

edited

Loading