Add azure app service plan module #555
Conversation
|
Please see #89 for the status on Terratest with Azure. In particular, we're starting some work around it, and when that work is done, we'll be able to come back to this PR! |
| @@ -0,0 +1 @@ | |||
| 0.12.24 | |||
There was a problem hiding this comment.
I think we should remove this. This is useful in principle, but since we constantly want to test with newer terraform versions for compatibility, this actually hinders our ability in practice.
| @@ -0,0 +1,53 @@ | |||
| # Terraform Azure App Service Plan Example | |||
There was a problem hiding this comment.
Reminder to update to the new README style.
| # --------------------------------------------------------------------------------------------------------------------- | ||
| # DEPLOY AN AZURE APP SERVICE PLAN | ||
| # This is an example of how to deploy an Azure App Service Plan | ||
| # --------------------------------------------------------------------------------------------------------------------- |
There was a problem hiding this comment.
NIT: add reference to the corresponding test like we are doing here: https://github.com/gruntwork-io/terratest/blob/master/examples/azure/terraform-azure-nsg-example/main.tf#L1
|
|
||
| # ------------------------------------------------------------------------------ | ||
| # CONFIGURE OUR AZURE CONNECTION | ||
| # ------------------------------------------------------------------------------ |
There was a problem hiding this comment.
NIT: bar length is not the same as the other blocks.
|
|
||
| tags = azurerm_resource_group.rg.tags | ||
| kind = var.kind | ||
| reserved = lower(var.kind) == "linux" ? true : lower(var.kind) == "windows" || lower(var.kind) == "app" ? false : var.reserved |
There was a problem hiding this comment.
NIT: We've found that combined conditionals are much easier to read when you break it up into multiple lines with parens:
| reserved = lower(var.kind) == "linux" ? true : lower(var.kind) == "windows" || lower(var.kind) == "app" ? false : var.reserved | |
| reserved = ( | |
| lower(var.kind) == "linux" | |
| ? true | |
| : ( | |
| lower(var.kind) == "windows" || lower(var.kind) == "app" | |
| ? false | |
| : var.reserved | |
| ) | |
| ) |
(NOTE: you might have to run terraform fmt if you commit the suggestion directly)
| @@ -0,0 +1,3 @@ | |||
| output "planids" { | |||
| value = "${azurerm_app_service_plan.plan.*.id}" | |||
There was a problem hiding this comment.
Do you intend this to be a list? Or a string? If string, might be better to join them using some delimitter.
| @@ -0,0 +1,55 @@ | |||
| variable "resourceGroupName" { | |||
There was a problem hiding this comment.
NIT:
- Need the standard variable blocks that break up into the sections Env Vars, Required Vars, and Optional Vars.
- The standard style order of variables we use is
description,type,defaultin that order.
| return plan | ||
| } | ||
|
|
||
| func getAppServicePlanE(planName string, resGroupName string, subscriptionID string) (*web.AppServicePlan, error) { |
There was a problem hiding this comment.
Expose as public as we discussed in the other PR.
| // GetAppServicePlan gets the AppServicePlan. | ||
| // planName - required to find the AppServicePlan. | ||
| // resGroupName - use an empty string if you have the AZURE_RES_GROUP_NAME environment variable set | ||
| // subscriptionId - use an empty string if you have the ARM_SUBSCRIPTION_ID environment variable set |
There was a problem hiding this comment.
NIT: Add the boilerplate comment about failing the test.
| var expectedSkuCapacity int32 | ||
| expectedSkuCapacity = 1 |
There was a problem hiding this comment.
NIT:
| var expectedSkuCapacity int32 | |
| expectedSkuCapacity = 1 | |
| expectedSkuCapacity := int32(1) |
|
@tsalright any issue if I pick up this PR and bring it up to date? |
|
@rguthriemsft go for it. I have been swamped and unable to visit this stuff in a while. Hoping to get back into this stuff in a couple months. |
|
@tsalright I think I have access to your fork, if not will ping you. I might pull the branch into our fork so I can run CI will let you know if I hit issues. |
|
@rguthriemsft sounds good. |
Adding to the Azure offerings in terratest #89
Adding App Service Plan module #548
Adding .terraform-version for anyone using tfenv to manage their terraform versions.