-
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 Resources: azurerm_app_service_slot
& azurerm_app_service_active_slot
#818
Conversation
… these service plans, updated documentation with this information as well.
@tombuildsstuff this PR has been verified by @jeffreyCline in our own environment. See test results link to Gist. |
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
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 @jeffreyCline
Thanks for this PR - I've taken a look through and left some comments inline, but this is looking really good. Most of the comments are really minor (whitespace) - the main thing we should change is updating the source_slot_name
field to app_service_slot_name
for consistency within Terraform - otherwise this looks good 👍
Thanks!
|
||
* `resource_group_name` - (Required) The name of the resource group in which to create the App Service Slot component. | ||
|
||
* `app_service_name` - (Required) The name of the App Service within which to create the App Service Slot. 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.
Can we make this (Required) The name of the App Service within which the Slot exists. Changing this forces a new resource to be created.
?
|
||
The following arguments are supported: | ||
|
||
* `resource_group_name` - (Required) The name of the resource group in which to create the App Service Slot component. |
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 make this (Required) The name of the Resource Group in which the App Service exists. Changing this forces a new resource to be created.
?
|
||
* `app_service_name` - (Required) The name of the App Service within which to create the App Service Slot. Changing this forces a new resource to be created. | ||
|
||
* `preserve_vnet` - (Required) `true` to preserve Virtual Network to the slot during swap; otherwise, `false`. |
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.
🤔 is there a use-case where we wouldn't want this to be true? I'm wondering if we default it to true and don't expose that for now?
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'm not sure, but it is exposed in models.go CsmSlotEntity struct so I exposed it to Terraform.
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.
taking a look at the Azure CLI and the Portal - neither of these expose this value; as such I think we can safely default this value to true
for the moment and not expose 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.
Ok, sounds good... I make that change.
|
||
* `preserve_vnet` - (Required) `true` to preserve Virtual Network to the slot during swap; otherwise, `false`. | ||
|
||
* `source_slot_name` - (Required) Source deployment slot for the swap operation. |
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 it might be worth naming this field app_service_slot_name
?
|
||
# azurerm_app_service_slot | ||
|
||
Swaps an App Service Slot (within an App Service) with the Production slot. |
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 make this: Promotes an App Service Slot to Production within an App Service
location = "${azurerm_resource_group.test.location}" | ||
resource_group_name = "${azurerm_resource_group.test.name}" | ||
app_service_plan_id = "${azurerm_app_service_plan.test.id}" | ||
app_service_name = "${azurerm_app_service.test.name}" |
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 could we sort out the spacing here?
location = "${azurerm_resource_group.test.location}" | ||
resource_group_name = "${azurerm_resource_group.test.name}" | ||
app_service_plan_id = "${azurerm_app_service_plan.test.id}" | ||
app_service_name = "${azurerm_app_service.test.name}" |
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 could we sort out the spacing here?
sku { | ||
tier = "Standard" | ||
size = "S1" | ||
} |
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 could we sort out the spacing here?
sku { | ||
tier = "Standard" | ||
size = "S1" | ||
} |
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 could we sort out the spacing here?
sku { | ||
tier = "Standard" | ||
size = "S1" | ||
} |
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 could we sort out the spacing here?
azurerm_app_service_slot
& azurerm_app_service_active_slot
(we generally use `{resourcename}1` so it's easier to tell what's an example)
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 @jeffreyCline
Thanks for pushing those updates - I've taken a look through and pushed a couple of updates to fix some documentation nits (I hope you don't mind!), but this otherwise LGTM 👍
Thanks!
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! |
This PR adds support for App Service Slot and Active Slot features to the Azure Terraform Provider.
Added Resources
resource_arm_app_service_active_slot
resource_arm_app_service_active_slot_test
resource_arm_app_service_slot
resource_arm_app_service_slot_test
app_service_slot.html.markdown
app_service_active_slot.html.markdown
Updated Resources
azurerm.erb
provider
Test Results