-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
update azurerm_postgresql_server/azurerm_mysql_server storage_mb validation bounds #1449
update azurerm_postgresql_server/azurerm_mysql_server storage_mb validation bounds #1449
Conversation
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 @gabinante,
Thank you for this contribution. Could we update the documentation to reflect the new limits? It'll LGTM once that is done.
@@ -135,7 +135,7 @@ func resourceArmMySqlServer() *schema.Resource { | |||
Type: schema.TypeInt, | |||
Required: true, | |||
ForceNew: true, | |||
ValidateFunc: validateIntBetweenDivisibleBy(5120, 1048576, 1024), | |||
ValidateFunc: validateIntBetweenDivisibleBy(5120, 4194304, 1024), |
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.
Shouldn't this also validate the SKU since only general purpose and memory optimized support this larger upper limit?
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.
Good catch, i agree
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.
@jeffreyCline I'm not super familiar with your conventions / golang style best practices. would you use a case/switch or an if here? Or is there a better option? Is there an example of another resource in the provider that does this?
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.
Here is an example of using the a CustomizeDiff
to ensure 1 of 2 properties is set.
In your case you can use this to get the storage_mb
property is within the bounds of what the current sku
allows.
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.
@gabinante I went ahead and added the validation code directly to your branch, hope that was ok. 😃
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 @gabinante
Thanks for the PR, it mostly looks good to go, but I left a comment. Can you please address it at your earliest convenience.
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.
The changes mostly look good. The test change requires you to change it for MemoryOptimized test. I have added the line link. Please update this.
@@ -386,7 +386,7 @@ resource "azurerm_postgresql_server" "test" { | |||
} | |||
|
|||
storage_profile { | |||
storage_mb = 947200 | |||
storage_mb = 4194304 |
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.
This test will fail. You might want to change the memoryOptimized test https://github.com/gabinante/terraform-provider-azurerm/blob/35a13bb4a1ffff476bd91f99391a31d42e60a48d/azurerm/resource_arm_postgresql_server_test.go#L455
@@ -84,7 +84,7 @@ The following arguments are supported: | |||
|
|||
`storage_profile` supports the following: | |||
|
|||
* `storage_mb` - (Required) Max storage allowed for a server, possible values are between `5120 MB` (5GB) and `1048576 MB` (1TB). The step for this value must be in `1024 MB` (1GB) increments. For more information see the [product documentation](https://docs.microsoft.com/en-us/rest/api/mysql/servers/create#StorageProfile). | |||
* `storage_mb` - (Required) Max storage allowed for a server, possible values are between `5120 MB` (5GB) and `4194304 MB` (4TB). The step for this value must be in `1024 MB` (1GB) increments. For more information see the [product documentation](https://docs.microsoft.com/en-us/rest/api/mysql/servers/create#StorageProfile). |
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.
Can we change this to be: ...possible values are between
5120 MB(5GB) and
1048576 MB(1TB) for the Basic SKU and between
5120 MB(5GB) and
4194304 MB (4TB) for General Purpose/Memory Optimized SKUs.
?
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.
sure, will add that to my commit with the conditional
@@ -83,7 +83,7 @@ The following arguments are supported: | |||
|
|||
`storage_profile` supports the following: | |||
|
|||
* `storage_mb` - (Required) Max storage allowed for a server, possible values are between `5120 MB` (5GB) and `1048576 MB` (1TB). The step for this value must be in `1024 MB` (1GB) increments. For more information see the [product documentation](https://docs.microsoft.com/en-us/rest/api/postgresql/servers/create#StorageProfile). | |||
* `storage_mb` - (Required) Max storage allowed for a server, possible values are between `5120 MB` (5GB) and `4194304 MB` (4TB). The step for this value must be in `1024 MB` (1GB) increments. For more information see the [product documentation](https://docs.microsoft.com/en-us/rest/api/postgresql/servers/create#StorageProfile). |
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.
Same as above.
Validated configuration since basic sku only supports up to 1 TB of storage.
Since the basic sku only allows up to 1 TB of storage
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 contributions @gabinante & @jeffreyCline ,
This LGTM 🙂
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
Fixes issue 1442 by updating the upper bound of PostgreSQL and MySQL database sizes to match the current maximum of 4 TB / 4194304 MB.
#1442
relevant azure documentation:
PostgreSQL - https://docs.microsoft.com/en-us/azure/postgresql/concepts-pricing-tiers
MySQL - https://docs.microsoft.com/en-us/azure/mysql/concepts-pricing-tiers
(fixes #1442)