-
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
azurerm_frontdoor
- Add support for backend_pools_send_receive_timeout_seconds
#6604
Conversation
azurerm_frontdoor
- Add support custom timeout for backend poolazurerm_frontdoor
- Add support custom timeout for backend pool
azurerm_frontdoor
- Add support custom timeout for backend poolazurerm_frontdoor
- Add support for backend_pools_send_receive_timeout_seconds
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 for the PR @WodansSon - aside from 2 comments about naming this LGTM 👍
@@ -67,6 +67,13 @@ func resourceArmFrontDoor() *schema.Resource { | |||
Required: true, | |||
}, | |||
|
|||
"backend_pools_send_receive_timeout_seconds": { |
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 backend_pool_send_receive_timeout_in_seconds
?
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 was originally going to call it that too, but after researching it, it's a global setting for all backend pools thus the plural. 🙂
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.
ahh see i would have assumed it was applying to all of them without the s, but there is another property backend_pool? which is different the the other back end pools i can see why heh
result := frontdoor.BackendPoolsSettings{ | ||
EnforceCertificateNameCheck: enforceCheck, | ||
SendRecvTimeoutSeconds: utils.Int32(backendPoolsSendReceiveTimeoutSeconds), | ||
} |
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.
Are there more settings here? would it make sense to move this into a backend_pool_configuration
block?
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.
Again, I was thinking the same thing... but I wanted to avoid a breaking change so I just put it at the top level with the other backend pools settings. Not sure if we should pull it into a block yet, if another setting is exposed in a future API I think it would be appropriate to make the change at that time... Maybe open a 3.0 breaking change issue for this to fix in the future?
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.
well we now have 2 so it makes sense to move into a block (especially when we have such long names) if we did it in 3.0 we would still ideally want to depreacate the old properties & support both so people have a migration path.
This has been released in version 2.8.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.8.0"
}
# ... other configuration ... |
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! |
(fixes #4558)