add ValidateFunc to resourceTFEPolicySet to catch syntax errors at tf plan time #168

mathyourlife-fitbit · 2020-05-07T06:33:49Z

Description

When using terraform, it can be frustrating to have a plan complete successfully as part of PR validation processes, only to have it fail after merged and attempting to apply. In this case, using a name for tfe_policy_set that includes a space would pass PR validation but throw an error on apply as the name should only contain letters, numbers, dash, and underscore as specified in the UI.

Testing plan

TESTARGS="-run TestAccTFEPolicySet_invalidname" make testacc

Output from acceptance tests

Apologies for only running a targeted test here instead of a full suite, but I do not yet have a full acceptance test environment setup.

$ TESTARGS="-run TestAccTFEPolicySet_invalidname" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEPolicySet_invalidname -timeout 15m
?   	github.com/terraform-providers/terraform-provider-tfe	[no test files]
=== RUN   TestAccTFEPolicySet_invalidname
--- PASS: TestAccTFEPolicySet_invalidname (0.54s)
PASS
ok  	github.com/terraform-providers/terraform-provider-tfe/tfe	(cached)
?   	github.com/terraform-providers/terraform-provider-tfe/version	[no test files]

… plan rather than error on apply

DeviaVir · 2020-05-23T08:49:23Z

tfe/resource_tfe_policy_set.go

@@ -22,6 +23,17 @@ func resourceTFEPolicySet() *schema.Resource {
 			"name": {
 				Type:     schema.TypeString,
 				Required: true,
+				ValidateFunc: func(v interface{}, k string) (ws []string, errs []error) {
+					value := v.(string)
+					matched, err := regexp.Match(`^[A-Za-z\d-_]+$`, []byte(value))


As described here https://www.terraform.io/docs/cloud/api/policy-sets.html

Thanks for submitting this PR. Having reviewed the contents I was wondering if any consideration was given to using one of the validators that are available in the validation package?

I think the use of the StringMatch validator will simplify things a great deal. As an example:

"name": { Type: schema.TypeString, Required: true, ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[\w\.\-]+$`), "The name of the policy set. Can only include letters, numbers, -, and _."), },

Yup, makes sense. I'll adjust. Thanks

Updated the validation function to the native validation call and updated the test to match the new error message

Tests still show passing

$ TESTARGS="-run TestAccTFEPolicySet_invalidname" make testacc ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEPolicySet_invalidname -timeout 15m ? github.com/terraform-providers/terraform-provider-tfe [no test files] === RUN TestAccTFEPolicySet_invalidname --- PASS: TestAccTFEPolicySet_invalidname (0.01s) PASS ok github.com/terraform-providers/terraform-provider-tfe/tfe (cached) ? github.com/terraform-providers/terraform-provider-tfe/version [no test files]

hcrhall

Happy to work on getting this merged. I recommend using the validation package within the SDK to ensure that naming requirements are fulfilled.

hcrhall · 2020-07-08T02:19:03Z

tfe/resource_tfe_policy_set.go

@@ -22,6 +23,17 @@ func resourceTFEPolicySet() *schema.Resource {
 			"name": {
 				Type:     schema.TypeString,
 				Required: true,
+				ValidateFunc: func(v interface{}, k string) (ws []string, errs []error) {
+					value := v.(string)
+					matched, err := regexp.Match(`^[A-Za-z\d-_]+$`, []byte(value))


Thanks for submitting this PR. Having reviewed the contents I was wondering if any consideration was given to using one of the validators that are available in the validation package?

I think the use of the StringMatch validator will simplify things a great deal. As an example:

"name": { Type: schema.TypeString, Required: true, ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[\w\.\-]+$`), "The name of the policy set. Can only include letters, numbers, -, and _."), },

…atch and update tests to match new error message

lafentres

Hi there. This is a great idea! Thank you for opening up a PR to add this.
Overall this looks good. I requested some small changes to make sure the regexp you've added matches exactly what the API expects and to make your changes consistent with the rest of the repo.

Once these changes are made, please tag @hcrhall and myself so we can run this through our internal CI and get it merged and released :]

I'm also happy to make these changes myself by opening up a branch containing your changes and my requested changes. Just let me know what works best for you!

lafentres · 2020-08-13T14:31:42Z

tfe/resource_tfe_policy_set.go

-				Required: true,
+				Type:         schema.TypeString,
+				Required:     true,
+				ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[\w\.\-]+$`), "The name of the policy set. Can only include letters, numbers, -, and _."),


This regex allows ., which isn't allowed in policy set names. The regexp in my suggestion below will match exactly what the API accepts.

I think it would be good to adjust the error message we return here as well, so it is consistent with other error messages. This should result in an error message that look something like this:

"invalid value for name (can only include letters, numbers, -, and _.)"

Suggested change

ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[\w\.\-]+$`), "The name of the policy set. Can only include letters, numbers, -, and _."),

ValidateFunc: validation.StringMatch(regexp.MustCompile(`\A[\w\_\-]+\z`), "can only include letters, numbers, -, and _."),

lafentres · 2020-08-13T14:38:36Z

tfe/resource_tfe_policy_set_test.go

+func TestAccTFEPolicySet_invalidname(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckTFEPolicySetDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config:      testAccTFEPolicySet_invalidname,
+				ExpectError: regexp.MustCompile(`The name of the policy set. Can only include letters, numbers, -, and _.`),
+			},
+		},
+	})
+}


There would need to be a change here as well, to make sure this lines up with my suggestions above.

This test should also be moved above the import test TestAccTFEPolicySetImport on line 295.

Suggested change

func TestAccTFEPolicySet_invalidname(t *testing.T) {

resource.Test(t, resource.TestCase{

PreCheck: func() { testAccPreCheck(t) },

Providers: testAccProviders,

CheckDestroy: testAccCheckTFEPolicySetDestroy,

Steps: []resource.TestStep{

{

Config: testAccTFEPolicySet_invalidname,

ExpectError: regexp.MustCompile(`The name of the policy set. Can only include letters, numbers, -, and _.`),

},

},

})

}

func TestAccTFEPolicySet_invalidName(t *testing.T) {

resource.Test(t, resource.TestCase{

PreCheck: func() { testAccPreCheck(t) },

Providers: testAccProviders,

CheckDestroy: testAccCheckTFEPolicySetDestroy,

Steps: []resource.TestStep{

{

Config: testAccTFEPolicySet_invalidName,

ExpectError: regexp.MustCompile(`can only include letters, numbers, -, and _.`),

},

},

})

}

lafentres · 2020-08-13T14:39:05Z

tfe/resource_tfe_policy_set_test.go

+const testAccTFEPolicySet_invalidname = `
+resource "tfe_organization" "foobar" {
+  name  = "tst-terraform"
+  email = "[email protected]"
+}
+
+resource "tfe_sentinel_policy" "foo" {
+  name         = "policy-foo"
+  policy       = "main = rule { true }"
+  organization = "${tfe_organization.foobar.id}"
+}
+
+resource "tfe_policy_set" "foobar" {
+  name         = "not the right format"
+  description  = "Policy Set"
+  organization = "${tfe_organization.foobar.id}"
+  policy_ids   = ["${tfe_sentinel_policy.foo.id}"]
+}`


Small change here as well, for consistency.

Suggested change

const testAccTFEPolicySet_invalidname = `

resource "tfe_organization" "foobar" {

name = "tst-terraform"

email = "[email protected]"

}

resource "tfe_sentinel_policy" "foo" {

name = "policy-foo"

policy = "main = rule { true }"

organization = "${tfe_organization.foobar.id}"

}

resource "tfe_policy_set" "foobar" {

name = "not the right format"

description = "Policy Set"

organization = "${tfe_organization.foobar.id}"

policy_ids = ["${tfe_sentinel_policy.foo.id}"]

}`

const testAccTFEPolicySet_invalidName = `

resource "tfe_organization" "foobar" {

name = "tst-terraform"

email = "[email protected]"

}

resource "tfe_sentinel_policy" "foo" {

name = "policy-foo"

policy = "main = rule { true }"

organization = "${tfe_organization.foobar.id}"

}

resource "tfe_policy_set" "foobar" {

name = "not the right format"

description = "Policy Set"

organization = "${tfe_organization.foobar.id}"

policy_ids = ["${tfe_sentinel_policy.foo.id}"]

}`

mathyourlife-fitbit · 2020-08-14T01:24:54Z

Thanks for the detailed review @lafentres . Changes implemented. cc @hcrhall

Tests pass

$ TESTARGS="-run TestAccTFEPolicySet_invalidName" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEPolicySet_invalidName -timeout 15m
?   	github.com/terraform-providers/terraform-provider-tfe	[no test files]
=== RUN   TestAccTFEPolicySet_invalidName
--- PASS: TestAccTFEPolicySet_invalidName (0.01s)
PASS
ok  	github.com/terraform-providers/terraform-provider-tfe/tfe	(cached)
?   	github.com/terraform-providers/terraform-provider-tfe/version	[no test files]

lafentres · 2020-08-18T22:38:50Z

Thanks @mathyourlife-fitbit! I've started the process of getting this through CI in #205 and will be working on getting this ready for merge this week.

lafentres

✨ 🎉 ✨

lafentres · 2020-08-19T15:24:05Z

@mathyourlife-fitbit this has been released! It is available now as part of 0.21.0

mathyourlife-fitbit · 2020-08-20T17:17:31Z

thanks for you help @lafentres @hcrhall 🥇

mathyourlife-fitbit added 3 commits May 7, 2020 02:03

add ValidateFunc to resourceTFEPolicySet to catch syntax errors at tf…

6c452a0

… plan rather than error on apply

adding acceptance test

1ceb22c

add back required reference resources to test

928c623

ghost added the size/S label May 7, 2020

DeviaVir reviewed May 23, 2020

View reviewed changes

DeviaVir approved these changes May 23, 2020

View reviewed changes

hcrhall suggested changes Jul 8, 2020

View reviewed changes

update resourceTFEPolicySet.ValidateFunc to native validation.StringM…

d614b60

…atch and update tests to match new error message

mathyourlife-fitbit requested a review from hcrhall July 31, 2020 00:39

lafentres requested changes Aug 13, 2020

View reviewed changes

pr feedback on regex match and naming consistency

6699f88

lafentres approved these changes Aug 19, 2020

View reviewed changes

lafentres merged commit 0798563 into hashicorp:master Aug 19, 2020

mathyourlife-fitbit deleted the tfe_policy_set-name-validation branch August 20, 2020 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ValidateFunc to resourceTFEPolicySet to catch syntax errors at tf plan time #168

add ValidateFunc to resourceTFEPolicySet to catch syntax errors at tf plan time #168

mathyourlife-fitbit commented May 7, 2020

DeviaVir May 23, 2020

hcrhall Jul 8, 2020

mathyourlife-fitbit Jul 30, 2020

mathyourlife-fitbit Jul 30, 2020

mathyourlife-fitbit Jul 30, 2020

hcrhall left a comment

hcrhall Jul 8, 2020

lafentres left a comment •

edited

Loading

lafentres Aug 13, 2020

lafentres Aug 13, 2020

lafentres Aug 13, 2020

mathyourlife-fitbit commented Aug 14, 2020 •

edited

Loading

lafentres commented Aug 18, 2020

lafentres left a comment

lafentres commented Aug 19, 2020

mathyourlife-fitbit commented Aug 20, 2020

	ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[\w\.\-]+$`), "The name of the policy set. Can only include letters, numbers, -, and _."),
	ValidateFunc: validation.StringMatch(regexp.MustCompile(`\A[\w\_\-]+\z`), "can only include letters, numbers, -, and _."),

add ValidateFunc to resourceTFEPolicySet to catch syntax errors at tf plan time #168

add ValidateFunc to resourceTFEPolicySet to catch syntax errors at tf plan time #168

Conversation

mathyourlife-fitbit commented May 7, 2020

Description

Testing plan

Output from acceptance tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcrhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafentres left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathyourlife-fitbit commented Aug 14, 2020 • edited Loading

lafentres commented Aug 18, 2020

lafentres left a comment

Choose a reason for hiding this comment

lafentres commented Aug 19, 2020

mathyourlife-fitbit commented Aug 20, 2020

lafentres left a comment •

edited

Loading

mathyourlife-fitbit commented Aug 14, 2020 •

edited

Loading