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 Beta resource: azurerm_windows_function_app #14247

Merged
merged 18 commits into from
Dec 8, 2021

Conversation

jackofallops
Copy link
Member

No description provided.

katbyte
katbyte previously requested changes Dec 5, 2021
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @jackofallops - initial review with comments left inline

Comment on lines +468 to +475
"scm_use_main_ip_restriction": {
Type: pluginsdk.TypeBool,
Optional: true,
Default: false,
Description: "Should the Windows Function App `ip_restriction` configuration be used for the SCM also.",
},

"scm_ip_restriction": IpRestrictionSchema(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

should these properties conflict?

Copy link
Member Author

Choose a reason for hiding this comment

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

The service accepts and updates this even if the ScmIPSecurityRestrictionsUseMain is true. There may be use-cases where this is necessary?

Description: "The number of pre-warmed instances for this function app. Only affects apps on an Elastic Premium plan.",
},

"remote_debugging": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be

Suggested change
"remote_debugging": {
"remote_debugging_enabled": {

Copy link
Member Author

Choose a reason for hiding this comment

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

This is consistent with the Windows Function App, but it is more in keeping with the provider, so I'll update both to the suggested value.

Description: "Should Remote Debugging be enabled. Defaults to `false`.",
},

"remote_debugging_version": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we include what version this referse to?

Suggested change
"remote_debugging_version": {
"remote_debugging_visualstudio_version": {

Copy link
Member Author

Choose a reason for hiding this comment

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

The possible values include the prefix VS so I think including visualstudio in the property name may be confusing here, possibly leading to users expecting to use 2017, 2019 etc instead?

Description: "The amount of time in minutes that a node is unhealthy before being removed from the load balancer. Possible values are between `2` and `10`. Defaults to `10`. Only valid in conjunction with `health_check_path`",
},

"number_of_workers": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as we use count normally should this be

Suggested change
"number_of_workers": {
"worker_count": {

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used in all the other resources in the beta also, so perhaps follow this PR with another that addresses it consistently throughout, rather than broadening the scope of change here?

Comment on lines +154 to +161
"client_cert_enabled": {
Type: pluginsdk.TypeBool,
Optional: true,
Default: false,
Description: "Should the function app use Client Certificates",
},

"client_cert_mode": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we expand cert?

Suggested change
"client_cert_enabled": {
Type: pluginsdk.TypeBool,
Optional: true,
Default: false,
Description: "Should the function app use Client Certificates",
},
"client_cert_mode": {
"client_certificate_enabled": {
Type: pluginsdk.TypeBool,
Optional: true,
Default: false,
Description: "Should the function app use Client Certificates",
},
"client_certificate_mode": {

Copy link
Member Author

Choose a reason for hiding this comment

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

As above, this affects all the other resources.

@jackofallops jackofallops dismissed katbyte’s stale review December 8, 2021 09:04

Discussed offline, followup PR to address across the RP.

@jackofallops jackofallops merged commit 9be4ae1 into main Dec 8, 2021
@jackofallops jackofallops deleted the f/beta_windows_function_app branch December 8, 2021 09:10
jackofallops added a commit that referenced this pull request Dec 8, 2021
@github-actions
Copy link

This functionality has been released in v2.89.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 10, 2022
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.

Linux/Windows App Service doesn't appear to support vnet_route_all site config
4 participants