-
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
Provision sample for ASP.NET on azure_rm_app_service #407
Provision sample for ASP.NET on azure_rm_app_service #407
Conversation
… as highlighted in #407 ``` $ acctests azurerm TestAzureRMAppServicePlanName_validation === RUN TestAzureRMAppServicePlanName_validation --- PASS: TestAzureRMAppServicePlanName_validation (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.029s $ acctests azurerm TestAzureRMAppServiceName_validation === RUN TestAzureRMAppServiceName_validation --- PASS: TestAzureRMAppServiceName_validation (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.019s ```
… as highlighted in #407 ``` $ acctests azurerm TestAzureRMAppServicePlanName_validation === RUN TestAzureRMAppServicePlanName_validation --- PASS: TestAzureRMAppServicePlanName_validation (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.029s $ acctests azurerm TestAzureRMAppServiceName_validation === RUN TestAzureRMAppServiceName_validation --- PASS: TestAzureRMAppServiceName_validation (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.019s ```
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 @pjmolina
Thanks for this PR - apologies for the delay in reviewing this! I've taken a look at this and left some comments in-line but this is off to a good start :)
Thanks!
examples/app-service-asp-net/app.tf
Outdated
} | ||
} | ||
|
||
# underscores not supported as app_service name -> if not: you will receive error 400 |
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.
Thanks for raising this - I've added some validation to the App Service / App Service Plan Resources to handle this in this PR
examples/app-service-asp-net/app.tf
Outdated
} | ||
|
||
output "adminUrl" { | ||
value = "https://${azurerm_app_service.common_service.name}.scm.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.
note: these two values (serviceUrl
and adminUrl
) assume we're running in Azure Public (and not Germany/Government/China etc) - I took a quick look into if we could expose these variables so this example could also work in those environments, but unfortunately those values aren't immediately available - so this should be fine :)
examples/app-service-asp-net/app.tf
Outdated
} | ||
|
||
resource "azurerm_resource_group" "g1" { | ||
name = "{$var.groupName}" |
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.
this should be ${var.groupName}
(I'll push a commit to fix this)
examples/app-service-asp-net/app.tf
Outdated
client_id = "${var.client_id}" | ||
client_secret = "${var.client_secret}" | ||
tenant_id = "${var.tenant_id}" | ||
} |
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 tend to leave this commented out in case people opt to use Azure CLI auth (I'll push a commit to amend this)
@@ -0,0 +1,45 @@ | |||
variable "groupName" { |
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.
minor I might suggest renaming this resource_group_name
to be clearer (and underscores for consistency) - (I'll push a commit for this)
default = "westeurope" | ||
} | ||
|
||
variable "webName" { |
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.
(same here) webName
-> web_name
(I'll push a commit for this)
examples/app-service-asp-net/app.tf
Outdated
# } | ||
|
||
provisioner "local-exec" { | ||
command = "curl -k -u ${var.deploy_user}:${var.deploy_pass} -X PUT --data-binary @${var.deployZipFile} https://${azurerm_app_service.common_service.name}.scm.azurewebsites.net/api/zip/site/wwwroot/" |
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.
out of interest, how are these values set? From memory these need to be set either in the portal/through the CLI before use, given Terraform currently doesn't support them and they're disabled by default?
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.
Thanks for the review Tom.
Yes, I didn't find a way yet to setup such deploy Azure credentials in a CLI/programmatic way.
As far as I know, them should be setup once in the Azure Portal per Azure Account.
Probably MS Azure guys can help with it.
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 @pjmolina
Apologies for the delayed response here!
In order for us to be able to ship this PR (without making the user having to navigate to the portal to enter the credentials) - I've pushed a commit to remove the publishing credentials snippet from the sample for the moment. Once support for configuring the App Service Publish Credentials is available through Terraform we can look into adding this back in - however for the moment this feels like the best compromise to ship this.
I've also opened #612 which exports the Default Hostname for the App Service - such that we can determine what it is (since azurewebsites.net
is only valid for Azure Public and not China/Germany/Government) - so once that's merged this PR should be good to merge :)
Hope that makes sense and you don't mind?
Thanks!
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.
Ok @tombuildsstuff I see your point.
A pity to loose the provisioner part because it is the main point of the example: provisioning the app, not just creating the bucket for it. It is true Microsoft should enable a way to setup the deployment credentials programmatically, without needing to enter manually in the portal.
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.
@pjmolina totally get that - we'll look to add it back in to the example once support for configuring the credentials is added to the Provider :)
This removes support for Publishing, since the SCM URL's aren't consistent across Sovereign Clouds (China/Germany/Govt etc) Switches to using the new `default_site_hostname` field introduced in hashicorp#612 rather than assuming what it is
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.
LGTM thanks for this - we'll merge this once #612 has been merged in 👍
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! |
Hi!
Added an example for provisioning an ASP.NET website into Azure App Service with terraform.