-
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
New Resource: azurerm_function_app
#647
Conversation
azurerm_function_app
resourceazurerm_function_app
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.
Hey @JunyiYi
Thanks for this PR - apologies for the delayed review here!
I've taken a look through and left some comments in-line but this is off to a good start - most of the issues are minor. The one worth mentioning is about the App Settings keys - we need to ensure that the Function App settings are appended to the user specified AppSettings before updating (and that they're removed before setting the value in the state) - otherwise users will have a perpetual diff.
Thanks!
} | ||
|
||
resource "azurerm_storage_account" "test" { | ||
name = "azure-functions-test-sa" |
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.
dashes aren't valid in storage account names, can we make this azurefunctestsa
|
||
* `name` - (Required) Specifies the name of the Azure Functions service. Changing this forces a new resource to be created. | ||
|
||
* `resource_group_name` - (Required) The name of the resource group in which to create the Azure Functions 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.
I think this'd read better if Azure Functions service
became just Function App
?
|
||
Manages an Azure Functions service. | ||
|
||
## Example Usage |
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 should make this Example Usage (within an App 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.
Maybe Example Usage (with App Service Plan)
is more accurate.
|
||
The following arguments are supported: | ||
|
||
* `name` - (Required) Specifies the name of the Azure Functions service. Changing this forces a new resource to be created. |
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 this'd read better if Azure Functions service
became just Function App
?
azurerm/resource_arm_function_app.go
Outdated
Type: schema.TypeBool, | ||
Optional: true, | ||
Default: true, | ||
ForceNew: true, |
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 link to the existing issue i.e.
// TODO: support Update once the API is fixed:
// https://github.com/Azure/azure-rest-api-specs/issues/1697
|
||
# azurerm_function_app | ||
|
||
Manages an Azure Functions 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.
can we update Azure Functions service
-> Function App
?
it could be handy to add a blue info box here about Consumption Plans until they can be created natively (e.g. once the computeMode
field has been added to the SDK) e.g.
-> **Note:** Function Apps can be deployed to either an App Service Plan or to a Consumption Plan. At this time it's possible to deploy a Function App into an existing Consumption Plan or a new/existing App Service Plan - however it's not currently possible to create a new Consumption Plan. Support for this will be added in the future, and in the interim can be achieved by using [the `azurerm_template_deployment` resource](template_deployment.html).
Can we also open an issue on the Rest API Specs Repository to get these fields added to the Swagger (so it can be added to the SDK)? I'm going to look into adding a Data Source for App Service Plans
since I could see referencing an existing App Service Plan being useful (especially in the context mentioned above)
page_title: "Azure Resource Manager: azurerm_function_app" | ||
sidebar_current: "docs-azurerm-resource-function-app-x" | ||
description: |- | ||
Manages an Azure Functions 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.
can we update Azure Functions service
-> Function App
?
--- | ||
layout: "azurerm" | ||
page_title: "Azure Resource Manager: azurerm_function_app" | ||
sidebar_current: "docs-azurerm-resource-function-app-x" |
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 need the -x
on the end of this (since there's no other resources with the prefix docs-azurerm-resource-function-app
at this time)
@@ -0,0 +1,79 @@ | |||
--- |
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 also add a link to this in the sidebar? I guess it should probably either be in the App Service
section or it's own Function App Resources
section? https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/website/azurerm.erb#L135
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" | ||
) | ||
|
||
func TestAccAzureRMFunctionApp_basic(t *testing.T) { |
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 add another acceptance test for going from V1 -> Beta
(and back)? It's possible to use multiple test steps to do this and assert on the value of the version
field in each step - as we do here: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_traffic_manager_endpoint_test.go#L111
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 thing.
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.
Hey @JunyiYi
Thanks for pushing those latest updates - I've taken a look and left some comments in-line but this LGTM.
I've pushed a couple of commits to add Import support and fix the documentation; but the tests pass:
$ acctests azurerm TestAccAzureRMFunctionApp_
=== RUN TestAccAzureRMFunctionApp_importBasic
--- PASS: TestAccAzureRMFunctionApp_importBasic (98.26s)
=== RUN TestAccAzureRMFunctionApp_basic
--- PASS: TestAccAzureRMFunctionApp_basic (124.61s)
=== RUN TestAccAzureRMFunctionApp_updateVersion
--- PASS: TestAccAzureRMFunctionApp_updateVersion (126.37s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 349.276s
Thanks!
location = "${azurerm_resource_group.test.location}" | ||
resource_group_name = "${azurerm_resource_group.test.name}" | ||
app_service_plan_id = "${azurerm_app_service_plan.test.id}" | ||
version = "%[4]s" |
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.
whilst this approach is preferable to reduce repetition - unfortunately there's a bug in golint
where variables of the wrong type aren't identified when used in this fashion (e.g. as %[1]s
vs %s
- I think we're safe to leave this for the moment but that's why we format the other way at the moment)
|
||
* `id` - The ID of the Function App | ||
|
||
* `default_hostname` - The default hostname associated with the Function App - such as `mysite.azurewebsites.net` |
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.
going to push an update to add Import documentation 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.
(done)
|
||
* `app_settings` - (Optional) A key-value pair of App Settings. | ||
|
||
* `enabled` - (Optional) Is the Azure Function service enabled? Changing this forces a new resource to be created. |
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.
going to push an update to change this to Function App
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.
(done)
(after testing this - I've also made the field |
@JunyiYi thanks for all the work on this - this LGTM 👍 |
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! |
primary_connection_string
andsecondary_connection_string
toazurerm_storage_account
resourceazurerm_function_app
resource to the provider