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

Refactor: mssql_elasticpool max_size_bytes to max_size_gb #2695

Merged
merged 46 commits into from
Feb 5, 2019

Conversation

WodansSon
Copy link
Collaborator

@WodansSon WodansSon commented Jan 17, 2019

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)

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @jeffreyCline

Thanks for this PR :) I've taken a look through and left some comments inline but this mostly LGTM - if we can fix up the comments (and the tests pass) then we should be able to get this merged 👍

Thanks!

azurerm/helpers/azure/elasticpool.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/elasticpool.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/elasticpool.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/elasticpool.go Outdated Show resolved Hide resolved
Optional: true,
Computed: true,
ValidateFunc: validation.IntAtLeast(0),
ValidateFunc: validate.FloatAtLeast(0),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tombuildsstuff I see what you mean now... went ahead and added the secondary schema field, it now supports both attributes.

azurerm/resource_arm_mssql_elasticpool.go Outdated Show resolved Hide resolved
@WodansSon
Copy link
Collaborator Author

image

@tombuildsstuff tombuildsstuff modified the milestones: 2.0.0, 1.22.0 Jan 22, 2019
@WodansSon
Copy link
Collaborator Author

image

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.

Comments left inline and further details in slack

azurerm/resource_arm_mssql_elasticpool.go Outdated Show resolved Hide resolved
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤯

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

A note that these two conflict and why might be useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, fixed.

80: 4096,
}

func BasicGetMaxSizeGB(DTUs int) float64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a bit more specific of name:

Suggested change
func BasicGetMaxSizeGB(DTUs int) float64 {
func MSSQLElasticPoolBasicGetMaxSizeGB(DTUs int) float64 {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

"strings"
)

var basicDTUMaxGB = map[int]float64{
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Took a slightly different approach, updated the keys so I could hold all values in a single map.

80: 4096,
}

var businessCriticalGen4vCoreMaxGB = map[int]float64{
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, i wonder if nested maps make this a little easier to grok?

[family][gen][core] == maxsize, allows for less variables and functions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Fixed.

return validTier
}

func GetFamily(name string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function name is definitely to vague, we should make it clear its for elastic pool

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

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

Choose a reason for hiding this comment

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

Where is this used? i don't see it anywher

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not used anymore... I should have removed it, missed one!

}

// Basic Checks
if strings.EqualFold(name.(string), "BasicPool") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

This is very similar to the standard checks, could we combine these into a function and share some of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@WodansSon
Copy link
Collaborator Author

image

@WodansSon
Copy link
Collaborator Author

image

@WodansSon
Copy link
Collaborator Author

image

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 updates @jeffreyCline! LGTM now

@katbyte katbyte merged commit 2de47a0 into master Feb 5, 2019
@katbyte katbyte deleted the refactor-elasticpool-maxsize branch February 5, 2019 06:03
katbyte added a commit that referenced this pull request Feb 5, 2019
@ghost
Copy link

ghost commented Mar 7, 2019

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 ghost locked and limited conversation to collaborators Mar 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sku Name Parameter for azurerm_mssql_elasticpool Doesn't Accept 'StandardPool'
3 participants