Skip to content
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

Merged
merged 10 commits into from Jun 28, 2018

Conversation

gabinante
Copy link
Contributor

@gabinante gabinante commented Jun 26, 2018

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)

Copy link
Collaborator

@katbyte katbyte left a 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.

@katbyte katbyte changed the title Bugfix/database size update update azurerm_postgresql_server/azurerm_mysql_server storage_mb validation bounds Jun 26, 2018
@katbyte katbyte added this to the 1.8.0 milestone Jun 26, 2018
@@ -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),
Copy link
Collaborator

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, i agree

Copy link
Contributor Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabinante,

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.

Copy link
Collaborator

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. 😃

Copy link
Collaborator

@WodansSon WodansSon left a 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.

Copy link
Contributor

@VaijanathB VaijanathB left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -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).
Copy link
Collaborator

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) and1048576 MB(1TB) for the Basic SKU and between5120 MB(5GB) and4194304 MB (4TB) for General Purpose/Memory Optimized SKUs.?

Copy link
Contributor Author

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).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

WodansSon and others added 6 commits June 27, 2018 17:20
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
Copy link
Collaborator

@katbyte katbyte left a 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 🙂

@katbyte katbyte merged commit d3cdd17 into hashicorp:master Jun 28, 2018
katbyte added a commit that referenced this pull request Jun 28, 2018
@ghost
Copy link

ghost commented Mar 30, 2020

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!

@hashicorp hashicorp locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants