New Resource - azurerm_linux_function_app_flex_consumption
#28199
Conversation
github-actions
bot
added
documentation
service/app-service
size/XL
waiting-response
labels
|
@xiaxyi - thank you for this PR. Only note from me is we don't want customers to worry about operating system, with no plans for a Windows version of Flex Consumption in the foreseeable future. Can we please simplify the name and code not to have Linux in it for Flex Consumption? Other than that I'm happy with separating it out into its own resource type, thank you. |
|
Thanks @nzthiago for the suggestions, @jackofallops are you agree with the idea of removing the |
|
Following up on this one @jackofallops - from the Azure Functions PG we would prefer to drop 'linux' from the resource name. |
|
@jackofallops : Could you please update on @nzthiago 's suggestions? We are eagerly waiting for this actually. |
|
Happy New Year @jackofallops. Could you be able to check on the suggestions? |
|
@xiaxyi please go ahead and remove |
There was a problem hiding this comment.
@xiaxyi, as a heads up this will require a few rounds of reviews, but I've made a first pass through to call out some consistency and style choices. Could you fix those up? I can then go over this in detail with our App Service SME.
Thanks!
| AppCommandLine string `tfschema:"app_command_line"` | ||
| ApiDefinition string `tfschema:"api_definition_url"` | ||
| ApiManagementConfigId string `tfschema:"api_management_api_id"` | ||
| AppInsightsInstrumentationKey string `tfschema:"application_insights_key"` // App Insights Instrumentation Key |
There was a problem hiding this comment.
Can we remove this comment here since it isn't really alluding to anything helpful (the name is already descriptive)
| AppInsightsInstrumentationKey string `tfschema:"application_insights_key"` // App Insights Instrumentation Key | |
| AppInsightsInstrumentationKey string `tfschema:"application_insights_key"` |
| Http2Enabled bool `tfschema:"http2_enabled"` | ||
| IpRestriction []IpRestriction `tfschema:"ip_restriction"` | ||
| IpRestrictionDefaultAction string `tfschema:"ip_restriction_default_action"` | ||
| LoadBalancing string `tfschema:"load_balancing_mode"` // TODO - Valid for FunctionApps? |
There was a problem hiding this comment.
Is this comment relevant here?
| LoadBalancing string `tfschema:"load_balancing_mode"` // TODO - Valid for FunctionApps? | |
| LoadBalancing string `tfschema:"load_balancing_mode"` |
| RuntimeScaleMonitoring bool `tfschema:"runtime_scale_monitoring_enabled"` | ||
| ScmIpRestriction []IpRestriction `tfschema:"scm_ip_restriction"` | ||
| ScmIpRestrictionDefaultAction string `tfschema:"scm_ip_restriction_default_action"` | ||
| ScmType string `tfschema:"scm_type"` // Computed? |
There was a problem hiding this comment.
Same here
| ScmType string `tfschema:"scm_type"` // Computed? | |
| ScmType string `tfschema:"scm_type"` |
| Optional: true, | ||
| Description: "The path to be checked for this function app health.", | ||
| RequiredWith: func() []string { | ||
| if features.FourPointOhBeta() { |
There was a problem hiding this comment.
4.0 has been shipped so we shouldn't be using this flag anywhere anymore
| Computed: !features.FourPointOhBeta(), | ||
| ValidateFunc: validation.IntBetween(2, 10), | ||
| RequiredWith: func() []string { | ||
| if features.FourPointOhBeta() { |
| if existing.Model == nil || existing.Model.Properties == nil { | ||
| return fmt.Errorf("reading of Linux %s for update", id) | ||
| } |
There was a problem hiding this comment.
For consistency these checks should be split and return the following error messages
| if existing.Model == nil || existing.Model.Properties == nil { | |
| return fmt.Errorf("reading of Linux %s for update", id) | |
| } | |
| if existing.Model == nil { | |
| return fmt.Errorf("reading %s: `model` was nil", id) | |
| } | |
| existing.Model.Properties == nil { | |
| return fmt.Errorf("reading %s: `properties` was nil", id) | |
| } |
| if response.WasNotFound(resp.HttpResponse) { | ||
| return pointer.To(false), nil | ||
| } | ||
| return nil, fmt.Errorf("retrieving Linux Function App %s: %+v", id, err) |
There was a problem hiding this comment.
We don't check for 404 in the exists check for the tests, any error when we try to get the resource in this check should be returned
| if response.WasNotFound(resp.HttpResponse) { | |
| return pointer.To(false), nil | |
| } | |
| return nil, fmt.Errorf("retrieving Linux Function App %s: %+v", id, err) | |
| return nil, fmt.Errorf("retrieving %s: %+v", id, err) |
| if response.WasNotFound(resp.HttpResponse) { | ||
| return pointer.To(false), nil | ||
| } |
There was a problem hiding this comment.
| if response.WasNotFound(resp.HttpResponse) { | |
| return pointer.To(false), nil | |
| } |
| if response.WasNotFound(resp.HttpResponse) { | ||
| return pointer.To(false), nil | ||
| } | ||
| return pointer.To(true), nil |
There was a problem hiding this comment.
The existence of a Model in the response confirms whether the resource exists or not
| return pointer.To(true), nil | |
| return pointer.To(resp.Model != nil), nil |
| %s | ||
|
|
||
| resource "azurerm_linux_function_app_flex_consumption" "test" { | ||
| name = "xiaxintest-LFA-tf%d" |
There was a problem hiding this comment.
Resource names in the acceptance tests should begin with acc
|
@stephybun Thanks for the review, I understand it will take server rounds of review, that's totally fine. I updated the pr based on the comments, feel free to let me know if anything needed. |
I believe, |
|
@stephybun I believe this is ready for the next round of review, thank you |
|
@stephybun checking in to progress with this PR, thank you |
| var _ sdk.ResourceWithStateMigration = FunctionAppFlexConsumptionResource{} | ||
|
|
||
| func (r FunctionAppFlexConsumptionResource) StateUpgraders() sdk.StateUpgradeData { | ||
| return sdk.StateUpgradeData{} | ||
| } |
There was a problem hiding this comment.
As this is a new resource it shouldn't have any state migrations so these lines can be removed
| var _ sdk.ResourceWithStateMigration = FunctionAppFlexConsumptionResource{} | |
| func (r FunctionAppFlexConsumptionResource) StateUpgraders() sdk.StateUpgradeData { | |
| return sdk.StateUpgradeData{} | |
| } |
| client := metadata.Client.AppService.WebAppsClient | ||
| resourcesClient := metadata.Client.AppService.ResourceProvidersClient | ||
| servicePlanClient := metadata.Client.AppService.ServicePlanClient | ||
| subscriptionId := metadata.Client.Account.SubscriptionId |
There was a problem hiding this comment.
These should be defined at the top of the Create function
|
|
||
| isFlexConsumptionSku := helpers.PlanIsFlexConsumption(planSKU) | ||
| if !isFlexConsumptionSku { | ||
| return fmt.Errorf("the sku name is %s which is not valid for flex consumption function app", *planSKU) |
There was a problem hiding this comment.
| return fmt.Errorf("the sku name is %s which is not valid for flex consumption function app", *planSKU) | |
| return fmt.Errorf("the sku name is %s which is not valid for a flex consumption function app", *planSKU) |
| if functionAppFlexConsumption.StorageUserAssignedIdentityID == "" { | ||
| return fmt.Errorf("the user assigned identity id must be specified when using the user assigned identity to access the storage account") | ||
| } else { | ||
| storageAuth.UserAssignedIdentityResourceId = &functionAppFlexConsumption.StorageUserAssignedIdentityID | ||
| } |
There was a problem hiding this comment.
This can be simplified
| if functionAppFlexConsumption.StorageUserAssignedIdentityID == "" { | |
| return fmt.Errorf("the user assigned identity id must be specified when using the user assigned identity to access the storage account") | |
| } else { | |
| storageAuth.UserAssignedIdentityResourceId = &functionAppFlexConsumption.StorageUserAssignedIdentityID | |
| } | |
| if functionAppFlexConsumption.StorageUserAssignedIdentityID == "" { | |
| return fmt.Errorf("the user assigned identity id must be specified when using the user assigned identity to access the storage account") | |
| } | |
| storageAuth.UserAssignedIdentityResourceId = &functionAppFlexConsumption.StorageUserAssignedIdentityID | |
|
|
||
| backupConfig, err := helpers.ExpandBackupConfig(functionAppFlexConsumption.Backup) | ||
| if err != nil { | ||
| return fmt.Errorf("expanding backup configuration for %s: %+v", id, err) |
There was a problem hiding this comment.
Error messages returned from expanding blocks should contain the block name in between backticks
| return fmt.Errorf("expanding backup configuration for %s: %+v", id, err) | |
| return fmt.Errorf("expanding `backup` for %s: %+v", id, err) |
| @@ -45,6 +45,7 @@ func (r Registration) Resources() []sdk.Resource { | |||
| FunctionAppHybridConnectionResource{}, | |||
| LinuxFunctionAppResource{}, | |||
| LinuxFunctionAppSlotResource{}, | |||
| FunctionAppFlexConsumptionResource{}, | |||
| Manages a Linux Function App Running in Flex Consumption Plan. | ||
| --- | ||
|
|
||
| # azurerm_function_app_flex_consumption | ||
|
|
||
| Manages a Linux Function App Running In The Flex Consumption Plan. |
There was a problem hiding this comment.
| Manages a Linux Function App Running in Flex Consumption Plan. | |
| --- | |
| # azurerm_function_app_flex_consumption | |
| Manages a Linux Function App Running In The Flex Consumption Plan. | |
| Manages a Function App Running on a Flex Consumption Plan. | |
| --- | |
| # azurerm_function_app_flex_consumption | |
| Manages a Function App Running on a Flex Consumption Plan. |
|
|
||
| The following arguments are supported: | ||
|
|
||
| * `location` - (Required) The Azure Region where the Linux Function App should exist. Changing this forces a new Linux Function App to be created. |
There was a problem hiding this comment.
Please remove all references to Linux in the docs
| * `location` - (Required) The Azure Region where the Linux Function App should exist. Changing this forces a new Linux Function App to be created. | |
| * `location` - (Required) The Azure Region where the Function App should exist. Changing this forces a new Function App to be created. |
|
|
||
| * `config_file_path` - (Optional) The path to the App Auth settings. | ||
|
|
||
| ~> **Note:** Relative Paths are evaluated from the Site Root directory. |
There was a problem hiding this comment.
Please be consistent with the casing in this documentation, some are NOTE and others are Note. Can you update them to follow the same casing?
| } | ||
|
|
||
| resource "azurerm_storage_container" "test" { | ||
| name = "blobforfc" |
There was a problem hiding this comment.
| name = "blobforfc" | |
| name = "acctestblobforfc" |
|
Just checking in on this. We're eager. 😁 |
Community Note
Description
To make a separate pr for this new feature, referring to the #27531
PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_resource- support for thething1property [GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.