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 Resources: azurerm_app_service_slot & azurerm_app_service_active_slot #818

Merged
merged 13 commits into from
Feb 13, 2018

Conversation

WodansSon
Copy link
Collaborator

@WodansSon WodansSon commented Feb 9, 2018

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

@metacpp
Copy link
Contributor

metacpp commented Feb 10, 2018

@tombuildsstuff this PR has been verified by @jeffreyCline in our own environment. See test results link to Gist.

Copy link
Contributor

@metacpp metacpp left a comment

Choose a reason for hiding this comment

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

LGTM

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

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?

Copy link
Collaborator Author

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.

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 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 :)

Copy link
Collaborator Author

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

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

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

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

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

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?

@tombuildsstuff tombuildsstuff modified the milestones: 1.1.3, 1.1.2 Feb 13, 2018
@tombuildsstuff tombuildsstuff changed the title Add Support for App Service Slot and Active Slot New Resources: azurerm_app_service_slot & azurerm_app_service_active_slot Feb 13, 2018
(we generally use `{resourcename}1` so it's easier to tell what's an example)
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 @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!

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Feb 13, 2018

All the App Service prefixed tests pass:

screen shot 2018-02-12 at 18 17 32

@tombuildsstuff tombuildsstuff merged commit cebe9f3 into master Feb 13, 2018
@tombuildsstuff tombuildsstuff deleted the f-app-service-slot branch February 13, 2018 02:33
tombuildsstuff added a commit that referenced this pull request Feb 13, 2018
@ghost
Copy link

ghost commented Mar 31, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 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.

4 participants