Issue #745, adds more options to azurerm_function_app #886
Conversation
There was a problem hiding this comment.
hey @cloudify
Thanks for this PR - I've taken a look and left some comments in-line, but this mostly LGTM. If we can confirm the defaults this should be good to merge :)
Thanks!
azurerm/resource_arm_function_app.go
Outdated
| "use_32_bit_worker_process": { | ||
| Type: schema.TypeBool, | ||
| Optional: true, | ||
| Default: false, |
There was a problem hiding this comment.
Based on this comment I believe this needs to be defaulted to true for Function Apps?
| @@ -87,7 +89,12 @@ The following arguments are supported: | |||
|
|
|||
| `site_config` supports the following: | |||
|
|
|||
| * `always_on` - (Optional) Should the app be loaded at all times? Defaults to `false`. | |||
| * `always_on` - (Optional) Should the Function App be loaded at all times? Defaults to `false`. | |||
| * `use_32_bit_worker_process` - (Optional) Should the Function App run in 32 bit mode, rather than 64 bit mode? | |||
There was a problem hiding this comment.
We should add "Defaults to value" (which should be true, based on above)
cloudify
force-pushed
the
745-function_app_site_config
branch
from
16539dd
to
1d65509
Compare
There was a problem hiding this comment.
hey @cloudify
Thanks for pushing those updates - I've taken another look through and if we can add an acceptance test setting a 64 bit worker process, this should be good to merge :)
Thanks!
| @@ -410,6 +441,14 @@ func flattenFunctionAppSiteConfig(input *web.SiteConfig) []interface{} { | |||
| result["always_on"] = *input.AlwaysOn | |||
| } | |||
|
|
|||
| if input.Use32BitWorkerProcess != nil { | |||
| result["use_32_bit_worker_process"] = *input.Use32BitWorkerProcess | |||
| } | |||
There was a problem hiding this comment.
could we add an acceptance test with the 64 bit worker process enabled? here's an example of how we're doing that
There was a problem hiding this comment.
so that we can get this merged, I've pushed an acceptance test for this - I hope you don't mind!
There was a problem hiding this comment.
not at all! thanks for pushing this, I got caught in higher priority tasks and couldn't focus on this now...
``` $ acctests azurerm TestAccAzureRMFunctionApp_3264bit === RUN TestAccAzureRMFunctionApp_3264bit --- PASS: TestAccAzureRMFunctionApp_3264bit (144.57s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 144.596s ```
There was a problem hiding this comment.
hey @cloudify
Thanks for pushing the latest updates - I've pushed a commit which adds an acceptance test for this, but this otherwise LGTM 👍
I'll kick off the test suite now, but this otherwise looks good to merge :)
Thanks!
|
Tests pass:
|
|
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 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
@tombuildsstuff how do you suggest I test this patch? should I create new tests or extend the current ones?