Skip to content
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

Merged
merged 11 commits into from
Jan 11, 2018
Merged

New Resource: azurerm_function_app #647

merged 11 commits into from
Jan 11, 2018

Conversation

JunyiYi
Copy link

@JunyiYi JunyiYi commented Dec 22, 2017

  • Add primary_connection_string and secondary_connection_string to azurerm_storage_account resource
  • Add azurerm_function_app resource to the provider
  • Add/Update documentations and tests

@JunyiYi JunyiYi changed the title Introduce azurerm_function_app resource Introduce azurerm_function_app resource Dec 22, 2017
@JunyiYi JunyiYi requested a review from metacpp December 22, 2017 01:43
@tombuildsstuff tombuildsstuff changed the title Introduce azurerm_function_app resource New Resource: azurerm_function_app Jan 4, 2018
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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"
Copy link
Contributor

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.
Copy link
Contributor

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
Copy link
Contributor

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)

Copy link
Author

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.
Copy link
Contributor

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?

Type: schema.TypeBool,
Optional: true,
Default: true,
ForceNew: true,
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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"
Copy link
Contributor

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 @@
---
Copy link
Contributor

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) {
Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing.

@tombuildsstuff tombuildsstuff modified the milestones: Future, 1.0.2 Jan 10, 2018
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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"
Copy link
Contributor

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 golintwhere 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`
Copy link
Contributor

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

Copy link
Contributor

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.
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(done)

@tombuildsstuff tombuildsstuff modified the milestones: 1.0.2, 1.0.1 Jan 11, 2018
@tombuildsstuff
Copy link
Contributor

(after testing this - I've also made the field storage_connection_string sensitive, since it's a connection string)

@tombuildsstuff
Copy link
Contributor

@JunyiYi thanks for all the work on this - this LGTM 👍

@tombuildsstuff
Copy link
Contributor

Whilst verifying this I've ended up pushing a couple of tests to add/update App Settings & Tags - which pass:

screen shot 2018-01-11 at 16 00 21

@tombuildsstuff tombuildsstuff merged commit 870709c into master Jan 11, 2018
@tombuildsstuff tombuildsstuff deleted the rm_functions_app branch January 11, 2018 16:01
tombuildsstuff added a commit that referenced this pull request Jan 11, 2018
@ghost
Copy link

ghost commented Apr 1, 2020

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 ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants