add check for functionApp kind and dynamic sku tier - "azurerm_app_service_plan" #7971
Conversation
There was a problem hiding this comment.
hi @njuCZ
Thanks for this PR - apologies for the delayed review here!
I've taken a look through and left some comments inline - if we can fix those up then we should be able to take another look
Thanks!
| @@ -117,7 +117,7 @@ The following arguments are supported: | |||
|
|
|||
| ~> **NOTE:** Attaching to an App Service Environment requires the App Service Plan use a `Premium` SKU (when using an ASEv1) and the `Isolated` SKU (for an ASEv2). | |||
|
|
|||
| * `reserved` - (Optional) Is this App Service Plan `Reserved`. Defaults to `false`. | |||
| * `reserved` - (Optional) Specifies whether it's Linux Platform. Should be `true` If Linux app service plan, false otherwise. Defaults to `false`. | |||
There was a problem hiding this comment.
this isn't just for Linux - this specifies if the App Service Plan should be always running or start/stopped on demand
There was a problem hiding this comment.
This field name is very misleading. According to https://docs.microsoft.com/en-us/rest/api/appservice/appserviceplans/createorupdate#request-body, it says If Linux app service plan true, false otherwise..
According to the implementation of azure.cli: https://github.com/Azure/azure-cli/blob/dev/src/azure-cli/azure/cli/command_modules/appservice/custom.py#L1648
users could specify the flag "--is-linux", and azure.cli will convert it to reserved field.
I have also emailed the service team, they says "reserved" means the OS type, "kind" is just a ARM level field, it's only used to drive the UX. It's not needed when creating. Azure.cli doesn't have the argument "kind".
Currently I think it's a little messy for resource "azurerm_app_service_plan" to have both field "reserved" and "kind", though it's the fault of service api definition. There seems a issue fired by a member of the service team: #5584
what's more, the OS type of app_service and function_app is defined by the app service plan, but for now both resource have "os_type" field, it should be "computed: true".
I wonder could we fix these in version 3 ?
| @@ -197,6 +197,14 @@ func resourceArmAppServicePlanCreateUpdate(d *schema.ResourceData, meta interfac | |||
| return fmt.Errorf("`reserved` has to be set to false when kind is set to `Windows`") | |||
| } | |||
|
|
|||
| if strings.EqualFold(*sku.Tier, "Dynamic") && !strings.EqualFold(kind, "FunctionApp") { | |||
| return fmt.Errorf("`kind` has to be set to `FunctionApp` when `sku.tier` is `Dynamic`") | |||
| } | |||
There was a problem hiding this comment.
isn't Dynamic available for Consumption Plan Web Apps too?
There was a problem hiding this comment.
this PR is just a workaround. kind should not be input by user, the backend service will calculate the value for kind. But to avoid breaking change, and to make user use it correctly, I added this constraints.
the backend service will always change the kind to functionapp if sku tier is dynamic. will change kind to elastic if sku tier is ElasticPremium.
| @@ -197,6 +197,14 @@ func resourceArmAppServicePlanCreateUpdate(d *schema.ResourceData, meta interfac | |||
| return fmt.Errorf("`reserved` has to be set to false when kind is set to `Windows`") | |||
| } | |||
|
|
|||
| if strings.EqualFold(*sku.Tier, "Dynamic") && !strings.EqualFold(kind, "FunctionApp") { | |||
There was a problem hiding this comment.
there's a crash here if sku.Tier is nil
| return fmt.Errorf("`kind` has to be set to `FunctionApp` when `sku.tier` is `Dynamic`") | ||
| } | ||
|
|
||
| if strings.EqualFold(*sku.Tier, "ElasticPremium") && !strings.EqualFold(kind, "elastic") { |
There was a problem hiding this comment.
there's a crash here if sku.Tier is nil
| } | ||
|
|
||
| if strings.EqualFold(*sku.Tier, "ElasticPremium") && !strings.EqualFold(kind, "elastic") { | ||
| return fmt.Errorf("`kind` has to be set to `elastic` when `sku.tier` is `ElasticPremium`") |
There was a problem hiding this comment.
can we add a test to check this?
| @@ -197,6 +197,14 @@ func resourceArmAppServicePlanCreateUpdate(d *schema.ResourceData, meta interfac | |||
| return fmt.Errorf("`reserved` has to be set to false when kind is set to `Windows`") | |||
| } | |||
|
|
|||
| if strings.EqualFold(*sku.Tier, "Dynamic") && !strings.EqualFold(kind, "FunctionApp") { | |||
| return fmt.Errorf("`kind` has to be set to `FunctionApp` when `sku.tier` is `Dynamic`") | |||
There was a problem hiding this comment.
can we add a test to check this?
|
@tombuildsstuff I have replied some of your questions. After we have draw the conclusion, I would fix the rest suggestions. |
|
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! |
ghost
locked as resolved and limited conversation to collaborators
fix #4396
fix #5971
functionappif sku tier isdynamic. will change kind toelasticif sku tier isElasticPremium. add check to avoid such config.reserved