Refactor: mssql_elasticpool max_size_bytes to max_size_gb #2695
Conversation
| Optional: true, | ||
| Computed: true, | ||
| ValidateFunc: validation.IntAtLeast(0), | ||
| ValidateFunc: validate.FloatAtLeast(0), | ||
| }, |
There was a problem hiding this comment.
In general we’d use a secondary schema field with the old deprecated name to allow a migration path here (since this is a breaking change) but since this is a major version upgrade and there’s going to be some breaking changes perhaps this is ok to skip that? 🤔
There was a problem hiding this comment.
Yeah, I thought it would be OK to skip that in this case since max_size_bytes hasn't been released to the wild as of yet.
There was a problem hiding this comment.
@tombuildsstuff I see what you mean now... went ahead and added the secondary schema field, it now supports both attributes.
|
|
|
|
| @@ -352,7 +475,14 @@ func resourceArmMsSqlElasticPoolRead(d *schema.ResourceData, meta interface{}) e | |||
| } | |||
|
|
|||
| if properties := resp.ElasticPoolProperties; properties != nil { | |||
| d.Set("max_size_bytes", properties.MaxSizeBytes) | |||
| // Basic tier does not return max_size_bytes, so we need to skip setting this | |||
| @@ -64,6 +64,8 @@ The following arguments are supported: | |||
|
|
|||
| * `per_database_settings` - (Required) A `per_database_settings` block as defined below. | |||
|
|
|||
| * `max_size_gb` - (Optional) The max data size of the elastic pool in gigabytes. | |||
|
|
|||
| * `max_size_bytes` - (Optional) The max data size of the elastic pool in bytes. | |||
|
|
|||
There was a problem hiding this comment.
A note that these two conflict and why might be useful.
There was a problem hiding this comment.
Good point, fixed.
azurerm/helpers/azure/elasticpool.go
Outdated
| 80: 4096, | ||
| } | ||
|
|
||
| func BasicGetMaxSizeGB(DTUs int) float64 { |
There was a problem hiding this comment.
This should be a bit more specific of name:
| func BasicGetMaxSizeGB(DTUs int) float64 { | |
| func MSSQLElasticPoolBasicGetMaxSizeGB(DTUs int) float64 { |
azurerm/helpers/azure/elasticpool.go
Outdated
| "strings" | ||
| ) | ||
|
|
||
| var basicDTUMaxGB = map[int]float64{ |
There was a problem hiding this comment.
Would a map of maps make this any easier?
DTUMaxGB = map[string]map[int]float64{
"basic" : {
50: 4.8828125,
100: 9.765625,
200: 19.53125,
...
},
"standard" : {
...
}
then you get Get:
```go
max := MSSQLElasticPoolGetMaxSizeGB(family, dtu)There was a problem hiding this comment.
Took a slightly different approach, updated the keys so I could hold all values in a single map.
azurerm/helpers/azure/elasticpool.go
Outdated
| 80: 4096, | ||
| } | ||
|
|
||
| var businessCriticalGen4vCoreMaxGB = map[int]float64{ |
There was a problem hiding this comment.
same here, i wonder if nested maps make this a little easier to grok?
[family][gen][core] == maxsize, allows for less variables and functions
azurerm/helpers/azure/elasticpool.go
Outdated
| return validTier | ||
| } | ||
|
|
||
| func GetFamily(name string) string { |
There was a problem hiding this comment.
This function name is definitely to vague, we should make it clear its for elastic pool
azurerm/helpers/suppress/string.go
Outdated
| @@ -9,3 +9,11 @@ import ( | |||
| func CaseDifference(_, old, new string, _ *schema.ResourceData) bool { | |||
| return strings.EqualFold(old, new) | |||
| } | |||
|
|
|||
| func IgnoreIfNotSet(_, old, new string, _ *schema.ResourceData) bool { | |||
There was a problem hiding this comment.
Where is this used? i don't see it anywher
There was a problem hiding this comment.
It's not used anymore... I should have removed it, missed one!
| } | ||
|
|
||
| // Basic Checks | ||
| if strings.EqualFold(name.(string), "BasicPool") { |
There was a problem hiding this comment.
Given that this must be a specific value maybe it would be best to just not allow the user to set it, or mark it as computed and populate it ourselves with a default?
There was a problem hiding this comment.
Talked about this in slack, going to leave this as is for... reasons.
| if capacity.(int) > 80 { | ||
| return fmt.Errorf("BusinessCritical pricing tier only supports upto 80 vCores") | ||
| // Premium Checks | ||
| if strings.EqualFold(name.(string), "PremiumPool") { |
There was a problem hiding this comment.
This is very similar to the standard checks, could we combine these into a function and share some of this?
|
|
|
|
|
|
|
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
I have added a ton of validation logic to the resource since the API is not friendly with the error message when a setting is wrong. The resource is very finicky about what values are accepted with different SKU's and if any of them are wrong it will fail with an internal server error.
(fixes #2622)