-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add name validation function, standardise validation functions used #164
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package google | ||
|
||
import ( | ||
"fmt" | ||
"github.com/hashicorp/terraform/helper/schema" | ||
"regexp" | ||
) | ||
|
||
func validateGCPName(v interface{}, k string) (ws []string, errors []error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this also return schema.SchemaValidationFunc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, better suggestion - make the other function return (ws []string, errors []error) (not a func) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There are similar functions to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spoke in person; I understand the differing signatures here. I'm fine with the code as is. |
||
re := `^(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?)$` | ||
return validateRegexp(re)(v, k) | ||
} | ||
|
||
func validateRegexp(re string) schema.SchemaValidateFunc { | ||
return func(v interface{}, k string) (ws []string, errors []error) { | ||
value := v.(string) | ||
if !regexp.MustCompile(re).MatchString(value) { | ||
errors = append(errors, fmt.Errorf( | ||
"%q (%q) doesn't match regexp %q", k, value, re)) | ||
} | ||
|
||
return | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
package google | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
) | ||
|
||
func TestValidateGCPName(t *testing.T) { | ||
x := []GCPNameTestCase{ | ||
// No errors | ||
{TestName: "basic", Value: "foobar"}, | ||
{TestName: "with numbers", Value: "foobar123"}, | ||
{TestName: "short", Value: "f"}, | ||
{TestName: "long", Value: "foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoo"}, | ||
{TestName: "has a hyphen", Value: "foo-bar"}, | ||
|
||
// With errors | ||
{TestName: "empty", Value: "", ExpectError: true}, | ||
{TestName: "starts with a capital", Value: "Foobar", ExpectError: true}, | ||
{TestName: "starts with a number", Value: "1foobar", ExpectError: true}, | ||
{TestName: "has an underscore", Value: "foo_bar", ExpectError: true}, | ||
{TestName: "too long", Value: "foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoobarfoob", ExpectError: true}, | ||
} | ||
|
||
es := testGCPNames(x) | ||
if len(es) > 0 { | ||
t.Errorf("Failed to validate GCP names: %v", es) | ||
} | ||
} | ||
|
||
type GCPNameTestCase struct { | ||
TestName string | ||
Value string | ||
ExpectError bool | ||
} | ||
|
||
func testGCPNames(cases []GCPNameTestCase) []error { | ||
es := make([]error, 0) | ||
for _, c := range cases { | ||
es = append(es, testGCPName(c)...) | ||
} | ||
|
||
return es | ||
} | ||
|
||
func testGCPName(testCase GCPNameTestCase) []error { | ||
_, es := validateGCPName(testCase.Value, testCase.TestName) | ||
if testCase.ExpectError { | ||
if len(es) > 0 { | ||
return nil | ||
} else { | ||
return []error{fmt.Errorf("Didn't see expected error in case \"%s\" with string \"%s\"", testCase.TestName, testCase.Value)} | ||
} | ||
} | ||
|
||
return es | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM