-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
azurerm_api_management_api - deprecate sku
in favour of the sku_name
property
#3154
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"capacity": { | ||
Type: schema.TypeInt, | ||
Required: true, | ||
ValidateFunc: validation.IntAtLeast(0), | ||
ValidateFunc: validation.IntAtLeast(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this value changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the SDK implementation:
// Capacity - Capacity of the SKU (number of deployed units of the SKU). The default value is 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that says default not minimum? thou it does make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤷♀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was more this would break anyone using 0 currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted code and made it like it was, to avoid potential impact to existing users...
Type: schema.TypeString, | ||
Optional: true, | ||
ConflictsWith: []string{"sku"}, | ||
ValidateFunc: azure.SkuNameStringInSlice([]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seems rather specific to this service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the line of code...
@@ -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 == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in other PR, we can just set both and simplify this logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now set both and removed the logic...
@@ -113,6 +154,10 @@ func TestAccAzureRMApiManagementApi_soapPassthrough(t *testing.T) { | |||
ResourceName: resourceName, | |||
ImportState: true, | |||
ImportStateVerify: true, | |||
ImportStateVerifyIgnore: []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -139,6 +184,8 @@ func TestAccAzureRMApiManagementApi_importSwagger(t *testing.T) { | |||
ImportState: true, | |||
ImportStateVerify: true, | |||
ImportStateVerifyIgnore: []string{ | |||
"sku", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -168,6 +215,8 @@ func TestAccAzureRMApiManagementApi_importWsdl(t *testing.T) { | |||
ImportState: true, | |||
ImportStateVerify: true, | |||
ImportStateVerifyIgnore: []string{ | |||
"sku", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And so on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -30,6 +31,58 @@ func TestAccAzureRMApiManagement_basic(t *testing.T) { | |||
ResourceName: resourceName, | |||
ImportState: true, | |||
ImportStateVerify: true, | |||
ImportStateVerifyIgnore: []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we mark this as deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revisions @jeffreyCline, Overall looking pretty good and i've just left a couple comments inline.
"capacity": { | ||
Type: schema.TypeInt, | ||
Required: true, | ||
ValidateFunc: validation.IntAtLeast(0), | ||
ValidateFunc: validation.IntAtLeast(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that says default not minimum? thou it does make sense
Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revisions, aside from a couple comments LGTM 👍
Computed: true, | ||
}, | ||
}, | ||
Elem: &schema.Resource{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have some trailing whitespace here
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor:
// Remove in 2.0 | |
// todo Remove in 2.0 |
@@ -40,23 +40,29 @@ func dataSourceApiManagementService() *schema.Resource { | |||
Computed: true, | |||
}, | |||
|
|||
// Remove in 2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor:
// Remove in 2.0 | |
// todo Remove in 2.0 |
"capacity": { | ||
Type: schema.TypeInt, | ||
Required: true, | ||
ValidateFunc: validation.IntAtLeast(0), | ||
ValidateFunc: validation.IntAtLeast(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤷♀
"capacity": { | ||
Type: schema.TypeInt, | ||
Required: true, | ||
ValidateFunc: validation.IntAtLeast(0), | ||
ValidateFunc: validation.IntAtLeast(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was more this would break anyone using 0 currently
@@ -36,6 +37,50 @@ func TestAccAzureRMApiManagement_basic(t *testing.T) { | |||
}) | |||
} | |||
|
|||
// Remove in 2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Remove in 2.0 | |
// todo Remove in 2.0 |
}) | ||
} | ||
|
||
// Remove in 2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Remove in 2.0 | |
// todo Remove in 2.0 |
* `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`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to escape the _ as its all in italics now? or is it just the guthub viewer here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
* `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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add this to the upgrading to 2.0 guide found in the guides directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure... added both the data source and the resource.
sku
in favour of the sku_name
property
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 ... |
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! |
Continuation of addressing issue #1500, Flatten the
SKU
block into a single attribute and unifying the handling ofSKU
across all resources within the Azure Terraform provider.Example: