-
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
Replacement Service: appservice
#12132
Conversation
5bf652e
to
e4074cb
Compare
d53ea91
to
94dbf06
Compare
837f73d
to
e32a27f
Compare
51532d6
to
71ab73c
Compare
71ab73c
to
040b837
Compare
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 @jackofallops - comments left inline
Optional: true, | ||
Elem: &pluginsdk.Schema{ | ||
Type: pluginsdk.TypeString, | ||
}, |
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.
should we at last validate this is not an empty string?
Type: pluginsdk.TypeList, | ||
Optional: true, | ||
Elem: &pluginsdk.Schema{Type: pluginsdk.TypeString}, | ||
}, |
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.
should we validate this is at least not empty??
ValidateFunc: webValidate.AppServiceEnvironmentID, // TODO - Bring over to this service | ||
}, | ||
|
||
"per_site_scaling": { |
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.
should this be
"per_site_scaling": { | |
"per_site_scaling_enabled": { |
azurerm/internal/services/appservice/serviceplan/service_plan_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/appservice/sourcecontrol/source_control_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/appservice/sourcecontrol/source_control_resource.go
Outdated
Show resolved
Hide resolved
64806d7
to
c98872b
Compare
c90a3fe
to
27f4ffd
Compare
|
||
func (r AppServiceSourceControlResource) Create() sdk.ResourceFunc { | ||
return sdk.ResourceFunc{ | ||
Timeout: 30 * time.Minute, |
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.
should we be passing this through from somewhere?
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.
Not to the best of my knowledge, this is the Typed SDK equivalent of Create: pluginsdk.DefaultTimeout(30 * time.Minute)
, so should be defined here with the function rather than collected from elsewhere? Happy to discuss alternatives.
|
||
func (r AppServiceSourceControlResource) Read() sdk.ResourceFunc { | ||
return sdk.ResourceFunc{ | ||
Timeout: 5 * time.Minute, |
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.
and here
|
||
func (r AppServiceSourceControlTokenResource) Read() sdk.ResourceFunc { | ||
return sdk.ResourceFunc{ | ||
Timeout: 5 * time.Minute, |
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.
ect
|
||
* `scm_use_main_ip_restriction` - (Optional) Should the Linux Web App `ip_restriction` configuration be used for the SCM also. | ||
|
||
* `use_32_bit_worker` - (Optional) Should the Linux Web App use a 32-bit worker. |
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.
use seems redundant?
* `use_32_bit_worker` - (Optional) Should the Linux Web App use a 32-bit worker. | |
* `32_bit_worker` - (Optional) Should the Linux Web App use a 32-bit worker. |
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 was a carry over from the superseded resource, I'll update it 👍
|
||
* `os_type` - (Optional) The O/S type for the App Services to be hosted in this plan. Possible values include `Windows` (default), `Linux`, and `WindowsContainer`. | ||
|
||
* `per_site_scaling` - (Optional) Should Per Site Scaling be enabled. Defaults to `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.
* `per_site_scaling` - (Optional) Should Per Site Scaling be enabled. Defaults to `false`. | |
* `per_site_scaling_enabled` - (Optional) Should Per Site Scaling be enabled. Defaults to `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.
This was a carry over from the superseded resource, I'll update it 👍
91428d8
to
70cc7fa
Compare
3ec1e98
to
982ce4b
Compare
|
||
## App Service | ||
|
||
App Service, aka `web`, was one of the earliest supported services in the provider and has evolved significantly over time, both as a Service and the resources within the provider. Over that time feature additions, behavioural changes, and a host of other change have made the current resources increasingly difficult to maintain and update. We're taking the opportunity in 3.0 to rewrite this service from the ground up. |
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 there's an s missing here
App Service, aka `web`, was one of the earliest supported services in the provider and has evolved significantly over time, both as a Service and the resources within the provider. Over that time feature additions, behavioural changes, and a host of other change have made the current resources increasingly difficult to maintain and update. We're taking the opportunity in 3.0 to rewrite this service from the ground up. | |
App Service, aka `web`, was one of the earliest supported services in the provider and has evolved significantly over time, both as a Service and the resources within the provider. Over that time feature additions, behavioural changes, and a host of other changes have made the current resources increasingly difficult to maintain and update. We're taking the opportunity in 3.0 to rewrite this service from the ground up. |
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.
Aside from a couple minor comments this LGTM 🚀🚀🚀🚀
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR introduces a new Service that will, over time, replace the
Web
service resources. This first change brings several new resources:azurerm_windows_web_app
,azurerm_linux_web_app
, which are intended to replaceazurerm_web_app
with O/S specific resources to better support their respective configuration and property validation within the provider.azurerm_app_service_source_control
- a new meta resource to better support the Source Control configuration of Web Apps (and Function Apps later, when they are added).azurerm_source_control_token
- A replacement typed resource and data source for adding an Access Token to App Service (GtHub, BitBucket, OneDrive, or DropBox). Note: This is a contextual resource and is created for the account, identity, or principal that creates it. This means that the whichever identity that needs to access it for App Service Deployments must be the account to run the Terraform to create it.Still outstanding:
azurerm_app_service_source_control
resource - not working as intendedSwitch to local SDK (Nice to have)azurerm_app_service_github_token
to generic token as previous resourceNote:
These new features are the first part of what will become version 3.0 of the provider, so are only available when the appropriate environment variable is set, details will be included in the change notes, but setting
ARM_PROVIDER_THREEPOINTZERO_RESOURCES=true
will permit the use of these new resource.