Add name validation function, standardise validation functions used #164
Conversation
…alidation function.
google/validation.go
Outdated
| "regexp" | ||
| ) | ||
|
|
||
| func validateGCPName(v interface{}, k string) (ws []string, errors []error) { |
There was a problem hiding this comment.
Good candidate for unit testing here
Also what do you think about splitting it out into two functions - validateGCPName and validateRegexp (where validateGCPName is implemented in terms of validateRegexp)? I can see us needing regexs for other things later (for example, metadata keys)
| @@ -68,7 +69,7 @@ func dataSourceGoogleSignedUrl() *schema.Resource { | |||
| Type: schema.TypeString, | |||
| Optional: true, | |||
| Default: "GET", | |||
| ValidateFunc: validateHttpMethod, | |||
| ValidateFunc: validation.StringInSlice([]string{"GET", "HEAD", "PUT", "DELETE"}, true), | |||
There was a problem hiding this comment.
Could add our own validation function that is 'isHTTPVerb' (although given not all the http verbs are here, maybe that would be more confusing?) What do you think?
There was a problem hiding this comment.
Given that not all of the http verbs are here, I don't think we should add a custom validator yet; if there are several other validators with identical parameters in the future, that is something we should consider, though.
| "regexp" | ||
| ) | ||
|
|
||
| func validateGCPName(v interface{}, k string) (ws []string, errors []error) { |
There was a problem hiding this comment.
Shouldn't this also return schema.SchemaValidationFunc?
There was a problem hiding this comment.
Or, better suggestion - make the other function return (ws []string, errors []error) (not a func)
There was a problem hiding this comment.
validateGCPName is implicitly a schema.SchemaValidationFunc because of its signature; we want validateRegexp to validate a against a variable regex and not a fixed one like validateGCPName, so we pass that regex into a function that makes a schema.SchemaValidationFunc for us.
There are similar functions to validateRegexp living in the core Terraform repo's helper/validation package.
There was a problem hiding this comment.
Spoke in person; I understand the differing signatures here. I'm fine with the code as is.
…ashicorp#164) * Used standard validation functions where possible, added a GCP name validation function. * Add tests for GCP name, factor out a ValidateRegexp function. * make fmt
…_check (hashicorp#164) <!-- This change is generated by MagicModules. --> /cc @rileykarson
…_check (hashicorp#164) <!-- This change is generated by MagicModules. --> /cc @rileykarson
|
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! |
ghost
locked and limited conversation to collaborators
We have several validation functions that are just reimplementations of ones that live in core's
helper/validationpackage; let's use those instead.We define a separate validation function each time we want to validate the
namefield of a resource. Let's use a single one instead.These two resources had validation functions for
namethat validated only length and not format, but both follow the same rules as most names, a 1-63 character RFC1035 string. Errors will now surface at plan time instead of apply time for them.