HCS-1805: Add consul cluster vm size #77
Conversation
riddhi89
commented
Mar 18, 2021
•
riddhi89
commented
- Adds a size attribute to the consul cluster properties. This can be provided during creation to specify t-shirt size of each server.
- Updates the hcp-go-sdk version.
- Cleans up some tier related validation in favor of leveraging sdk provided validation.
There was a problem hiding this comment.
Everything looks good to me from the overall provider perspective! Thanks for looking into the enum validation pattern. I like that your approach keeps the API (by extension of the generated SDK) as the source of truth - definitely an improvement I'd like to add that to our style guide.
I'll leave it to another consul team member to verify the business logic is all good.
| sharedmodels "github.com/hashicorp/hcp-sdk-go/clients/cloud-shared/v1/models" | ||
| ) | ||
|
|
||
| // GetConsulClusterByID gets an Consul cluster by its ID | ||
| func GetConsulClusterByID(ctx context.Context, client *Client, loc *sharedmodels.HashicorpCloudLocationLocation, | ||
| consulClusterID string) (*consulmodels.HashicorpCloudConsul20200826Cluster, error) { | ||
| consulClusterID string) (*consulmodels.HashicorpCloudConsul20210204Cluster, error) { |
There was a problem hiding this comment.
oh gosh this is annoying to have to change everywhere. 😅 just wrote up an SDK ticket for seeing if we can strip out that version number, so these don't have to be updated every time the service version bumps
There was a problem hiding this comment.
Thank you! That would be great.
| @@ -37,20 +33,6 @@ var consulCusterResourceCloudProviders = []string{ | |||
| "aws", | |||
| } | |||
|
|
|||
| // consulClusterResourceTiers is the list of tiers | |||
internal/provider/validators.go
Outdated
| err := consulmodels.HashicorpCloudConsul20210204ClusterConfigTier(strings.ToUpper(v.(string))).Validate(strfmt.Default) | ||
| if err != nil { | ||
| expectedEnumList := regexp.MustCompile(`\[.*\]`).FindStringSubmatch(err.Error()) | ||
| msg := fmt.Sprintf("expected %v to be one of %v", v, expectedEnumList[0]) |
There was a problem hiding this comment.
A little confusing that expectedEnumList seems to be a list of lists 🤔 Any way to flatten?
There was a problem hiding this comment.
Agreed. Changed the regexp helper to extract the list directly.
| @@ -117,6 +117,11 @@ func dataSourceConsulCluster() *schema.Resource { | |||
| Type: schema.TypeString, | |||
| Computed: true, | |||
| }, | |||
| "size": { | |||
| Description: "The t-shirt size representation of each server VM that this Consul cluster\nis provisioned with. Valid options - x_small, small, medium, large.", | |||
There was a problem hiding this comment.
Nit: Was there a reason you needed the \n in the description here and in the resource? If it is being done to control where the line wraps, I think we would generally prefer to have the docs renderer handle this.
There was a problem hiding this comment.
Good catch. This was just a copy pasta of the desc from elsewhere. Removed.
| } | ||
| break | ||
| } | ||
| if err := d.Set("tier", strings.ToLower(string(cluster.Config.Tier))); err != nil { |
There was a problem hiding this comment.
Nit: Since you're already using DiffSuppressFunc, I don't think you need to normalize this with ToLower (same with size)
| @@ -106,3 +108,39 @@ func validateDatacenter(v interface{}, path cty.Path) diag.Diagnostics { | |||
|
|
|||
| return diagnostics | |||
| } | |||
|
|
|||
| func validateConsulClusterTier(v interface{}, path cty.Path) diag.Diagnostics { | |||
There was a problem hiding this comment.
Very cool pattern for these validate* functions, thanks for adding them (and tests)!
| @@ -135,6 +117,16 @@ func resourceConsulCluster() *schema.Resource { | |||
| Optional: true, | |||
| ForceNew: true, | |||
| }, | |||
| "size": { | |||
| Description: "The t-shirt size representation of each server VM that this Consul cluster\nis provisioned with. Valid options - x_small, small, medium, large.", | |||
There was a problem hiding this comment.
Trying to think of how we could ensure this description stays up-to-date with the enum values, but it looks like the SDK doesn't provide them in a list, just constants. 🤔
There was a problem hiding this comment.
Yea this was the other place I did not like the hard-coding :'( The compromise was at least the sdk validation will be our gate check. But, open to suggestions if any.
- Adds a size attribute to the consul cluster properties. This can be provided during creation to specify t-shirt size of the server. - Updates the hcp-go-sdk version. - Cleans up some tier related validation in favor of leveraging sdk provided validation.
Fix grammar Co-authored-by: Kelly Devlin <[email protected]>
Fix grammar Co-authored-by: Kelly Devlin <[email protected]>
