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.

Sign up for GitHub

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

Jump to bottom

provider/azurerm: Create azure App services #12001

Closed
wants to merge 7 commits into from
Closed

provider/azurerm: Create azure App services #12001

wants to merge 7 commits into from

Conversation

lstolyarov
Copy link
Contributor

@lstolyarov lstolyarov commented Feb 16, 2017 •
edited
Loading

Example usage:

resource "azurerm_server_farm" "server_farm" {
  name = "${var.server_farm_name}"
  location = "${var.location}"
  resource_group_name = "${azurerm_resource_group.resource_group.name}"
  tier = "S3"
}

resource "azurerm_web_app" "web_app" {
  name = "${var.web_app_name}"
  resource_group_name = "${azurerm_resource_group.resource_group.name}"
  server_farm_id = "${azurerm_server_farm.server_farm.id}"
  location = "${var.location}"
  always_on = "true"
}

@lstolyarov
Copy link
Contributor Author

@stack72 here is a pull request we discussed yesterday.

@lstolyarov lstolyarov mentioned this pull request Feb 16, 2017
Closed
@lstolyarov lstolyarov changed the title [WIP] provider/azurerm: Create azure App services provider/azurerm: Create azure App services Feb 27, 2017
@worldspawn
Copy link

Are there any plans to support controlling application settings, web slots, instance counts, etc?

@ghost
Copy link

ghost commented Mar 23, 2017

Yep same here. We use app services a lot and I'm surprised it's not in there yet.

tombuildsstuff
tombuildsstuff suggested changes Mar 23, 2017
View reviewed changes
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.

Hi @lstolyarov

Thanks for this contribution :)

Apologies for the delay in reviewing this - I've posted some comments inline.

I've previously attempted to add support for App Services / App Service Plans and encountered the same issues you've worked around in that the SDK currently doesn't match the API Response. In order to try and get that fixed in the official SDK, I ended up raising the following issues:

Given this is a bug in the API, it'd be great if we could switch over to using the official SDK instead of a fork - as once the API is fixed the SDK should start working automatically.

The other thing we need to consider is the naming - as far as I'm aware, Web Apps have had several names:

  • Azure Websites / Web Hosting Plans
  • Web Apps / Server Farms
  • App Services / App Service Plans

It'd be great if we could switch the resources over to using the latest marketing names - which in this case are App Service and App Service Plan respectively :)

Thanks!

builtin/providers/azurerm/provider.go Outdated
@@ -124,6 +124,9 @@ func Provider() terraform.ResourceProvider {
"azurerm_sql_database": resourceArmSqlDatabase(),
"azurerm_sql_firewall_rule": resourceArmSqlFirewallRule(),
"azurerm_sql_server": resourceArmSqlServer(),

"azurerm_web_app": resourceArmWebApp(),
"azurerm_server_farm": resourceArmServerFarm(),
Copy link
Contributor

Choose a reason for hiding this comment

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

There's been several names for this product (Websites, Web Apps and now App Services) - however it'd be good if we could use the latest name for each product here i.e.

azurerm_server_farm -> azurerm_app_service_plan
azurerm_web_app -> azurerm_app_service

Copy link
Contributor Author

@lstolyarov lstolyarov Apr 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Updated the naming conventions. It should align with Microsoft product names now.

builtin/providers/azurerm/config.go Outdated
@@ -25,6 +25,7 @@ import (
"github.com/Azure/go-autorest/autorest/azure"
"github.com/hashicorp/terraform/terraform"
riviera "github.com/jen20/riviera/azure"
"github.com/lstolyarov/azure-sdk-for-go/arm/web"
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking a look through the git log - it looks like you've encountered two issues in the SDK where the API is returning Integers instead of Strings for Enum values, which I've previously raised some tickets about:

Azure/azure-rest-api-specs#938
Azure/azure-rest-api-specs#841

Given the problem is bugs in the API, once the fix is deployed - the SDK should start working as intended. As such could we switch this over to using the Official SDK instead of a fork? (otherwise, when the fix is deployed, prior versions of Terraform will stop working)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched over to the official SDK as it is now working correctly.

builtin/providers/azurerm/resource_arm_server_farm.go Outdated
serverFarmEnvelope := web.ServerFarmWithRichSku{
Location: &location,
Sku: &sku,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be good to add the maximumNumberOfWorkers field here

builtin/providers/azurerm/resource_arm_server_farm.go Outdated

d.Set("name", name)
d.Set("resource_group_name", resGroup)

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to set the tier and location fields in the state here - or they'll perpetually have changes

builtin/providers/azurerm/resource_arm_web_app.go Outdated
"location": {
Type: schema.TypeString,
Required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

this can become "location": locationSchema(),

builtin/providers/azurerm/resource_arm_web_app.go Outdated
Default: "",
},
"force_dns_registration": {
Type: schema.TypeString,
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 be able to use TypeBool here?

builtin/providers/azurerm/resource_arm_web_app.go Outdated
"skip_custom_domain_verification": {
Type: schema.TypeString,
Optional: true,
Default: "",
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 be able to use TypeBool here?

builtin/providers/azurerm/resource_arm_web_app.go Outdated
"skip_dns_registration": {
Type: schema.TypeString,
Optional: true,
Default: "",
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 be able to use TypeBool here?

builtin/providers/azurerm/resource_arm_web_app.go Outdated
}

d.Set("name", name)
d.Set("resource_group_name", resGroup)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to set the other fields (e.g. location / skip_dns_registration etc.) in the state here

builtin/providers/azurerm/resource_arm_web_app.go Outdated
return &schema.Resource{
Create: resourceArmWebAppCreate,
Read: resourceArmWebAppRead,
Update: resourceArmWebAppCreate,
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: since this is both Creating and Updating Web Apps - it'd be good to rename this to CreateUpdate to match the other resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated for both App service and App service plan.

@tombuildsstuff tombuildsstuff added the waiting-response An issue/pull request is waiting for a response from the community label Mar 23, 2017
@lstolyarov lstolyarov closed this Apr 8, 2017
@lstolyarov lstolyarov force-pushed the azurerm/app-service-resource branch from 9c28f94 to 6ac0d4c Compare April 8, 2017 16:51
@lstolyarov lstolyarov reopened this Apr 8, 2017
@lstolyarov lstolyarov mentioned this pull request Apr 13, 2017
Closed
@AtwoodTM
Copy link

@tombuildsstuff , now that the Azure SDK has been fixed, any ETA on getting Terraform resource support for Azure App Services and App Service Plans soon? Thanks in advance.

@catsby catsby removed the waiting-response An issue/pull request is waiting for a response from the community label Apr 26, 2017
@tombuildsstuff tombuildsstuff mentioned this pull request Apr 26, 2017
Merged
21 tasks
@tombuildsstuff
Copy link
Contributor

Hi @lstolyarov / @AtwooTM

Thanks for splitting this PR out into two :)

Just to give an update here - this PR is blocked on the following PR's:

Once both of these are merged, we can proceed with this PR - apologies for the delay; and thanks for all the great work here :)

tombuildsstuff
tombuildsstuff suggested changes Apr 26, 2017
View reviewed changes
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.

Hi @lstolyarov

As mentioned in the other comment - thanks for all the effort here :)

I've taken another look through this PR and left some comments in-line. It'd be great to add some acceptance tests covering the App Service resource (I'm aware this'll need an App Service Plan, however we should be able to define the App Service Plan resource in code/HCL - which will allow the tests to run once #13634 is merged?)

Thanks :)

builtin/providers/azurerm/resource_arm_app_service.go Outdated
Optional: true,
Default: "",
},
"server_farm_id": {
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 rename this to app_service_plan_id?

builtin/providers/azurerm/resource_arm_app_service.go
Schema: map[string]*schema.Schema{
"resource_group_name": {
Type: schema.TypeString,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is a breaking change, this'll need a ForceNew: true here

builtin/providers/azurerm/resource_arm_app_service.go Outdated
},
"name": {
Type: schema.TypeString,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is a breaking change, this'll need a ForceNew: true here

builtin/providers/azurerm/resource_arm_app_service.go Outdated
siteProps := web.SiteProperties{
SiteConfig: &siteConfig,
}
if v, ok := d.GetOk("server_farm_id"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above - can we rename this to app_service_plan_id?

builtin/providers/azurerm/resource_arm_app_service.go
"ttl_in_seconds": {
Type: schema.TypeString,
Optional: true,
Default: "",
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 be better as an int, rather than a string - given we can validate it against the API docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ttl_in_seconds is passed in as a string

https://godoc.org/github.com/Azure/azure-sdk-for-go/arm/web#AppsClient.CreateOrUpdate

builtin/providers/azurerm/resource_arm_app_service.go Outdated
return err
}

log.Printf("[DEBUG] Reading web app details %s", id)
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 rename web app -> app service here?

builtin/providers/azurerm/resource_arm_app_service.go Outdated
d.SetId("")
return nil
}
return fmt.Errorf("Error making Read request on Azure Web App %s: %s", name, err)
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 rename Azure Web App -> AzureRM App Service Plan here?

builtin/providers/azurerm/resource_arm_app_service.go Outdated
}

d.Set("name", name)
d.Set("resource_group_name", resGroup)
Copy link
Contributor

Choose a reason for hiding this comment

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

the other fields need to be set here too:

  • always_on
  • tags
  • location
  • app_service_plan_id
  • ttl_in_seconds
  • force_dns_registration
  • skip_custom_domain_verification
  • skip_dns_registration

builtin/providers/azurerm/resource_arm_app_service.go Outdated

deleteMetrics := true
deleteEmptyServerFarm := true
skipDNSRegistration := true
Copy link
Contributor

Choose a reason for hiding this comment

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

given we're defaulting these values for the delete - should we do the same for the create?

builtin/providers/azurerm/resource_arm_app_service.go Outdated
resGroup := id.ResourceGroup
name := id.Path["sites"]

log.Printf("[DEBUG] Deleting web app %s: %s", resGroup, name)
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 rename web app -> app service?

@lstolyarov lstolyarov mentioned this pull request Jun 11, 2017
Merged
@lstolyarov lstolyarov closed this Jun 21, 2017
@lstolyarov
Copy link
Contributor Author

Migrated to hashicorp/terraform-provider-azurerm#2

@ghost
Copy link

ghost commented Apr 8, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff requested changes

Assignees

@tombuildsstuff tombuildsstuff

Labels
enhancement provider/azurerm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lstolyarov @worldspawn @AtwoodTM @tombuildsstuff @catsby @leostolyarov