`azurerm_windows_function_app`, `azurerm_linux_function_app` - adding property `website_content_share_over_vnet` #26646

xiaxyi · 2024-07-16T06:23:45Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”
azurerm_windows_function_app, azurerm_linux_function_app - adding property website_content_over_vnet

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

--- PASS: TestAccWindowsFunctionApp_websiteContentOverVnetUpdate (585.08s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/appservice 603.626s
--- PASS: TestAccLinuxFunctionApp_websiteContentOverVnetUpdate (643.45s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/appservice 648.859s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_windows_function_app, azurerm_linux_function_app - adding property website_content_over_vnet

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

…tion app

jackofallops

Thanks @xiaxyi - Some comments and changes to take a look at below. I think this needs a little re-work, particularly around the tests as both configurable options there are being used for this setting, so it's inconclusive if the new property is behaving / work as expected and will mask any conflicting behaviours with the older app_setting mechanism of enabling this behaviour?
Can you take a look, and I'll continue review.

Thanks!

jackofallops · 2024-07-22T12:07:16Z

internal/services/appservice/linux_function_app_resource.go

@@ -70,6 +70,7 @@ type LinuxFunctionAppModel struct {
 	PublishingDeployBasicAuthEnabled bool                                       `tfschema:"webdeploy_publish_basic_authentication_enabled"`
 	PublishingFTPBasicAuthEnabled    bool                                       `tfschema:"ftp_publish_basic_authentication_enabled"`
 	Identity                         []identity.ModelSystemAssignedUserAssigned `tfschema:"identity"`
+	VnetContentShareEnabled          bool                                       `tfschema:"website_content_over_vnet"`


Following contributor guidelines, this should be:

Suggested change

VnetContentShareEnabled bool `tfschema:"website_content_over_vnet"`

VnetContentShareEnabled bool `tfschema:"website_content_over_vnet_enabled"`

jackofallops · 2024-07-22T12:16:22Z

internal/services/appservice/linux_function_app_resource.go

+		"website_content_over_vnet": {
+			Type:        pluginsdk.TypeBool,
+			Optional:    true,
+			Default:     false,
+			Description: "Should the app scale enabled when storage account restricted to a virtual network? Defaults to `false`.",
+		},


As above, this should be website_content_over_vnet_enabled.

I also think the Description here needs re-wording, I'm not sure I understand what this has to do with app scale?

jackofallops · 2024-07-22T12:48:47Z

internal/services/appservice/linux_function_app_resource_test.go

+  app_settings = {
+    WEBSITE_CONTENTOVERVNET = "%s"
+    WEBSITE_CONTENTSHARE    = "shareforfa"
+  }
+
+  site_config {}
+  website_content_over_vnet = %t


We should not be setting both of these? If we're promoting this to be a top level attribute we should be excluding it's use in the app_settings map and managing that on the user's behalf as with the other examples of this behaviour.

jackofallops · 2024-07-22T12:49:05Z

internal/services/appservice/windows_function_app_resource.go

@@ -69,6 +69,7 @@ type WindowsFunctionAppModel struct {
 	ZipDeployFile                    string                                 `tfschema:"zip_deploy_file"`
 	PublishingDeployBasicAuthEnabled bool                                   `tfschema:"webdeploy_publish_basic_authentication_enabled"`
 	PublishingFTPBasicAuthEnabled    bool                                   `tfschema:"ftp_publish_basic_authentication_enabled"`
+	VnetContentShareEnabled          bool                                   `tfschema:"website_content_over_vnet"`


as above:

Suggested change

VnetContentShareEnabled bool `tfschema:"website_content_over_vnet"`

VnetContentShareEnabled bool `tfschema:"website_content_over_vnet_enabled"`

jackofallops · 2024-07-22T12:49:42Z

internal/services/appservice/windows_function_app_resource.go

+		"website_content_over_vnet": {
+			Type:        pluginsdk.TypeBool,
+			Optional:    true,
+			Default:     false,
+			Description: "Should the app scale enabled when storage account restricted to a virtual network? Defaults to `false`.",
+		},


as above:

Suggested change

"website_content_over_vnet": {

Type: pluginsdk.TypeBool,

Optional: true,

Default: false,

Description: "Should the app scale enabled when storage account restricted to a virtual network? Defaults to `false`.",

},

"website_content_over_vnet_enabled": {

Type: pluginsdk.TypeBool,

Optional: true,

Default: false,

Description: "Should the app scale enabled when storage account restricted to a virtual network? Defaults to `false`.",

},

Similarly, can you review the description here, it doesn't make sense to me?

jackofallops · 2024-07-22T12:50:48Z

internal/services/appservice/linux_function_app_resource.go

 					_, contentSharePresent := functionApp.AppSettings["WEBSITE_CONTENTSHARE"]
 					if _, contentShareConnectionStringPresent := functionApp.AppSettings["WEBSITE_CONTENTAZUREFILECONNECTIONSTRING"]; !contentShareConnectionStringPresent {
 						functionApp.AppSettings["WEBSITE_CONTENTAZUREFILECONNECTIONSTRING"] = storageString
 					}

 					if !contentSharePresent {
-						if contentOverVnetEnabled {
+						if contentOverVnetEnabled || contentOverVnetEnabledSiteSetting {
 							return fmt.Errorf("the app_setting WEBSITE_CONTENTSHARE must be specified and set to a valid share when WEBSITE_CONTENTOVERVNET is specified")


This error message needs additional clarification as there will be 2 ways this value can be true?

jackofallops · 2024-07-22T12:52:16Z

internal/services/appservice/windows_function_app_resource.go

 					_, contentSharePresent := functionApp.AppSettings["WEBSITE_CONTENTSHARE"]
 					if _, contentShareConnectionStringPresent := functionApp.AppSettings["WEBSITE_CONTENTAZUREFILECONNECTIONSTRING"]; !contentShareConnectionStringPresent {
 						functionApp.AppSettings["WEBSITE_CONTENTAZUREFILECONNECTIONSTRING"] = storageString
 					}

 					if !contentSharePresent {
-						if contentOverVnetEnabled {
+						if contentOverVnetEnabled || contentOverVnetEnabledSiteSetting {
 							return fmt.Errorf("the app_setting WEBSITE_CONTENTSHARE must be specified and set to a valid share when WEBSITE_CONTENTOVERVNET is specified")


As above, this needs to be clear about the property, not just the app_setting value.

jackofallops · 2024-07-22T12:55:19Z

website/docs/d/linux_function_app.html.markdown

+* `website_content_over_vnet` - Is function app scale enabled when the linked storage account is restricted to a virtual network? Defaults to `false`.
+
+!> **Note:** This property will replace the `WEBSITE_CONTENTOVERVNET` in `app_setting`. `website_content_over_vnet` is the recommended one despite both settings work currently.


This description doesn't sound like it matches what the setting does, it doesn't relate to app scale, rather if the storage account is accessed via VNet?

github-actions · 2024-08-26T06:03:18Z

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

adding website_content_over_vnet property

367747e

github-actions bot added documentation service/app-service size/L waiting-response labels Jul 16, 2024

add unparam linter

1e92ca4

github-actions bot removed the waiting-response label Jul 16, 2024

update the updateFunc logic and test cases for windows and linux Func…

cb7988a

…tion app

jackofallops requested changes Jul 22, 2024

View reviewed changes

github-actions bot added the waiting-response label Jul 22, 2024

github-actions bot added the stale label Aug 26, 2024

rcskosir added the enhancement label Oct 14, 2024

github-actions bot removed the stale label Oct 21, 2024

xiaxyi added 2 commits November 13, 2024 17:37

merging upstream and resolving conflicts

9b2127c

update pr based on comments

f808647

xiaxyi requested review from katbyte and a team as code owners November 13, 2024 10:36

github-actions bot removed the waiting-response label Nov 13, 2024

update windows test case

b99344c

xiaxyi changed the title ~~azurerm_windows_function_app, azurerm_linux_function_app - adding property website_content_over_vnet~~ azurerm_windows_function_app, azurerm_linux_function_app - adding property website_content_share_over_vnet Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_windows_function_app`, `azurerm_linux_function_app` - adding property `website_content_share_over_vnet` #26646

`azurerm_windows_function_app`, `azurerm_linux_function_app` - adding property `website_content_share_over_vnet` #26646

xiaxyi commented Jul 16, 2024 •

edited

Loading

jackofallops left a comment

jackofallops Jul 22, 2024

jackofallops Jul 22, 2024

jackofallops Jul 22, 2024

jackofallops Jul 22, 2024

jackofallops Jul 22, 2024

jackofallops Jul 22, 2024

jackofallops Jul 22, 2024

jackofallops Jul 22, 2024

github-actions bot commented Aug 26, 2024

	VnetContentShareEnabled bool `tfschema:"website_content_over_vnet"`
	VnetContentShareEnabled bool `tfschema:"website_content_over_vnet_enabled"`

		* `website_content_over_vnet` - Is function app scale enabled when the linked storage account is restricted to a virtual network? Defaults to `false`.

		!> Note: This property will replace the `WEBSITE_CONTENTOVERVNET` in `app_setting`. `website_content_over_vnet` is the recommended one despite both settings work currently.

azurerm_windows_function_app, azurerm_linux_function_app - adding property website_content_share_over_vnet #26646

Are you sure you want to change the base?

azurerm_windows_function_app, azurerm_linux_function_app - adding property website_content_share_over_vnet #26646

Conversation

xiaxyi commented Jul 16, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 26, 2024

`azurerm_windows_function_app`, `azurerm_linux_function_app` - adding property `website_content_share_over_vnet` #26646

`azurerm_windows_function_app`, `azurerm_linux_function_app` - adding property `website_content_share_over_vnet` #26646

xiaxyi commented Jul 16, 2024 •

edited

Loading