-
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
add zone_redundant as editable flag for mssql_elasticpool #3104
add zone_redundant as editable flag for mssql_elasticpool #3104
Conversation
46b705e
to
b434b8a
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.
Hi @bs-matil,
Thank you for the PR. I think we need to conditionally send the value to the API (see my comment for more details). Other then that i think this is looking pretty good.
1dd2394
to
336bd17
Compare
@katbyte I now embedded all requested changes. |
@@ -92,14 +92,14 @@ The following arguments are supported: | |||
|
|||
--- | |||
|
|||
* `zone_redundant` - (Optional) Whether or not this elastic pool is zone redundant. `tier` needs to be `Premium` for `DTU` based or `BusinessCritical` for `vCore` based `sku`. 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.
@katbyte should we leave the Defaults to false here? As its the API behavior anyways its still may a fair hint?
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 would leave it as yes that is what the api does by default.
336bd17
to
41884d4
Compare
41884d4
to
7dc548f
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 for making the changes so quickly!
LGTM 🙂
@@ -92,14 +92,14 @@ The following arguments are supported: | |||
|
|||
--- | |||
|
|||
* `zone_redundant` - (Optional) Whether or not this elastic pool is zone redundant. `tier` needs to be `Premium` for `DTU` based or `BusinessCritical` for `vCore` based `sku`. 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.
I would leave it as yes that is what the api does by default.
This has been released in version 1.24.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 = "~> 1.24.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! |
Hi,
we need zone_redundant elastic pools and therefore implemented the flag on the mssql_elasticpool resource.
Feel free to add any comments.