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

Adds validation to cloudflare_record to ensure TTL is not set while proxied is true #127

Merged
merged 6 commits into from
Oct 19, 2018

Conversation

mrparkers
Copy link
Contributor

Closes #126

Tests:

TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccCloudflareRecord_TtlValidation* -timeout 120m
?   	github.com/terraform-providers/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareRecord_TtlValidation
=== PAUSE TestAccCloudflareRecord_TtlValidation
=== RUN   TestAccCloudflareRecord_TtlValidationUpdate
=== PAUSE TestAccCloudflareRecord_TtlValidationUpdate
=== CONT  TestAccCloudflareRecord_TtlValidation
=== CONT  TestAccCloudflareRecord_TtlValidationUpdate
--- PASS: TestAccCloudflareRecord_TtlValidation (0.02s)
--- PASS: TestAccCloudflareRecord_TtlValidationUpdate (3.71s)
PASS
ok  	github.com/terraform-providers/terraform-provider-cloudflare/cloudflare	3.738s

@ghost ghost added the size/M label Oct 17, 2018
@jacobbednarz
Copy link
Member

Initially I was thinking we should rely on ValidateFunc here to enforce this at a schema level however after some fiddling around, it seems it only has access to the current key and not the rest of the schema 😞

I'm 👍 with this approach providing we update the error message to be more accurate.

@mrparkers
Copy link
Contributor Author

Yeah I agree that ValidateFunc could offer a lot more functionality. This is how I have seen it approached in other Terraform providers.

Also, that new "Suggested Change" feature is awesome. Thanks for not making me open my IDE to address your comments 😄

Copy link
Member

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Thanks a bunch @mrparkers 🍰

@mrparkers
Copy link
Contributor Author

I'm not sure I trust Travis since it reported a passing build when those tests probably should have failed. Here's a new snippet for test output with these changes:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccCloudflareRecord_TtlValidation* -timeout 120m
?   	github.com/terraform-providers/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareRecord_TtlValidation
=== PAUSE TestAccCloudflareRecord_TtlValidation
=== RUN   TestAccCloudflareRecord_TtlValidationUpdate
=== PAUSE TestAccCloudflareRecord_TtlValidationUpdate
=== CONT  TestAccCloudflareRecord_TtlValidation
=== CONT  TestAccCloudflareRecord_TtlValidationUpdate
--- PASS: TestAccCloudflareRecord_TtlValidation (0.03s)
--- PASS: TestAccCloudflareRecord_TtlValidationUpdate (4.27s)
PASS
ok  	github.com/terraform-providers/terraform-provider-cloudflare/cloudflare	4.292s

@jacobbednarz
Copy link
Member

Definitely worth digging into as I was thinking the same thing when the build came back green but had the incorrect assertions.

@patryk patryk merged commit 71c5e36 into cloudflare:master Oct 19, 2018
@patryk
Copy link
Contributor

patryk commented Oct 19, 2018

Thanks Michael and Jacob! We'll revisit tests if they become a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants