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

Add name validation function, standardise validation functions used #164

Merged
merged 3 commits into from
Jul 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions google/data_source_google_compute_zones.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"time"

"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
compute "google.golang.org/api/compute/v1"
)

Expand All @@ -24,15 +25,9 @@ func dataSourceGoogleComputeZones() *schema.Resource {
Elem: &schema.Schema{Type: schema.TypeString},
},
"status": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: func(v interface{}, k string) (ws []string, es []error) {
value := v.(string)
if value != "UP" && value != "DOWN" {
es = append(es, fmt.Errorf("%q can only be 'UP' or 'DOWN' (%q given)", k, value))
}
return
},
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice([]string{"UP", "DOWN"}, false),
},
},
}
Expand Down
12 changes: 2 additions & 10 deletions google/data_source_storage_object_signed_url.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/hashicorp/errwrap"
"github.com/hashicorp/terraform/helper/pathorcontents"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
"golang.org/x/oauth2/google"
"golang.org/x/oauth2/jwt"
)
Expand Down Expand Up @@ -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),
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

},
"path": &schema.Schema{
Type: schema.TypeString,
Expand All @@ -93,15 +94,6 @@ func validateExtensionHeaders(v interface{}, k string) (ws []string, errors []er
return
}

func validateHttpMethod(v interface{}, k string) (ws []string, errs []error) {
value := v.(string)
value = strings.ToUpper(value)
if value != "GET" && value != "HEAD" && value != "PUT" && value != "DELETE" {
errs = append(errs, errors.New("http_method must be one of [GET|HEAD|PUT|DELETE]"))
}
return
}

func dataSourceGoogleSignedUrlRead(d *schema.ResourceData, meta interface{}) error {
config := meta.(*Config)

Expand Down
17 changes: 4 additions & 13 deletions google/resource_compute_backend_bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package google
import (
"fmt"
"log"
"regexp"

"github.com/hashicorp/terraform/helper/schema"
"google.golang.org/api/compute/v1"
Expand All @@ -18,18 +17,10 @@ func resourceComputeBackendBucket() *schema.Resource {

Schema: map[string]*schema.Schema{
"name": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
re := `^(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?)$`
if !regexp.MustCompile(re).MatchString(value) {
errors = append(errors, fmt.Errorf(
"%q (%q) doesn't match regexp %q", k, value, re))
}
return
},
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validateGCPName,
},

"bucket_name": &schema.Schema{
Expand Down
17 changes: 4 additions & 13 deletions google/resource_compute_backend_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bytes"
"fmt"
"log"
"regexp"

"github.com/hashicorp/terraform/helper/hashcode"
"github.com/hashicorp/terraform/helper/schema"
Expand All @@ -23,18 +22,10 @@ func resourceComputeBackendService() *schema.Resource {

Schema: map[string]*schema.Schema{
"name": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
re := `^(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?)$`
if !regexp.MustCompile(re).MatchString(value) {
errors = append(errors, fmt.Errorf(
"%q (%q) doesn't match regexp %q", k, value, re))
}
return
},
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validateGCPName,
},

"health_checks": &schema.Schema{
Expand Down
10 changes: 1 addition & 9 deletions google/resource_compute_instance_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,7 @@ func resourceComputeInstanceTemplate() *schema.Resource {
Computed: true,
ForceNew: true,
ConflictsWith: []string{"name_prefix"},
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
// https://cloud.google.com/compute/docs/reference/latest/instanceTemplates#resource
value := v.(string)
if len(value) > 63 {
errors = append(errors, fmt.Errorf(
"%q cannot be longer than 63 characters", k))
}
return
},
ValidateFunc: validateGCPName,
},

"name_prefix": &schema.Schema{
Expand Down
17 changes: 4 additions & 13 deletions google/resource_compute_region_backend_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bytes"
"fmt"
"log"
"regexp"

"github.com/hashicorp/terraform/helper/hashcode"
"github.com/hashicorp/terraform/helper/schema"
Expand All @@ -20,18 +19,10 @@ func resourceComputeRegionBackendService() *schema.Resource {

Schema: map[string]*schema.Schema{
"name": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
re := `^(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?)$`
if !regexp.MustCompile(re).MatchString(value) {
errors = append(errors, fmt.Errorf(
"%q (%q) doesn't match regexp %q", k, value, re))
}
return
},
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validateGCPName,
},

"health_checks": &schema.Schema{
Expand Down
10 changes: 1 addition & 9 deletions google/resource_compute_ssl_certificate.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,7 @@ func resourceComputeSslCertificate() *schema.Resource {
Computed: true,
ForceNew: true,
ConflictsWith: []string{"name_prefix"},
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
// https://cloud.google.com/compute/docs/reference/latest/sslCertificates#resource
value := v.(string)
if len(value) > 63 {
errors = append(errors, fmt.Errorf(
"%q cannot be longer than 63 characters", k))
}
return
},
ValidateFunc: validateGCPName,
},

"name_prefix": &schema.Schema{
Expand Down
26 changes: 7 additions & 19 deletions google/resource_sql_database_instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"

"google.golang.org/api/googleapi"
"google.golang.org/api/sqladmin/v1beta4"
Expand Down Expand Up @@ -167,18 +168,14 @@ func resourceSqlDatabaseInstance() *schema.Resource {
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"day": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
return validateNumericRange(v, k, 1, 7)
},
Type: schema.TypeInt,
Optional: true,
ValidateFunc: validation.IntBetween(1, 7),
},
"hour": &schema.Schema{
Type: schema.TypeInt,
Optional: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
return validateNumericRange(v, k, 0, 23)
},
Type: schema.TypeInt,
Optional: true,
ValidateFunc: validation.IntBetween(0, 23),
},
"update_track": &schema.Schema{
Type: schema.TypeString,
Expand Down Expand Up @@ -1179,15 +1176,6 @@ func resourceSqlDatabaseInstanceDelete(d *schema.ResourceData, meta interface{})
return nil
}

func validateNumericRange(v interface{}, k string, min int, max int) (ws []string, errors []error) {
value := v.(int)
if min > value || value > max {
errors = append(errors, fmt.Errorf(
"%q outside range %d-%d.", k, min, max))
}
return
}

func instanceMutexKey(project, instance_name string) string {
return fmt.Sprintf("google-sql-database-instance-%s-%s", project, instance_name)
}
24 changes: 24 additions & 0 deletions google/validation.go
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this also return schema.SchemaValidationFunc?

Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
}
}
57 changes: 57 additions & 0 deletions google/validation_test.go
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
}