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

Exposing default_mode of WAF Rule through cloudflare_waf_rules data source #980

Closed
wants to merge 5 commits into from

Conversation

JoseRoman
Copy link

@jacobbednarz
Copy link
Member

seems reasonable 👍 can you please add test coverage for this new attribute?

@JoseRoman
Copy link
Author

@jacobbednarz Of course, I can add some tests for this.

  • What is the process of running the acceptance tests locally? Do I have to create a test Cloudflare account to be able to run them?
  • This is a read-only field (similar to allowed_modes) that is not being filtered, do we still write tests for these?

@jacobbednarz
Copy link
Member

What is the process of running the acceptance tests locally? Do I have to create a test Cloudflare account to be able to run them?

There is a section in the README on developing the provider which will step you through testing. Yes, you'll need a Cloudflare account as the tests create real resources.

This is a read-only field (similar to allowed_modes) that is not being filtered, do we still write tests for these?

https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/cloudflare/data_source_waf_rules_test.go#L13 is an example of an existing data_source_waf_rules test which you could copy, tweak and confirm your new fields are usable (either through confirming them being written to the state file or in another resource).

Config: testAccCloudflareWAFRulesConfig(zoneID, map[string]string{}, rnd),
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudflareWAFRulesDataSourceID(name),
resource.TestCheckResourceAttrSet(name, "rules.#"),
Copy link
Member

Choose a reason for hiding this comment

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

i'm afraid this assertion doesn't do assert anything useful for us here. this test is identical to some others and doesn't actually cover the functionality you've added.

perhaps have a look at testAccCheckCloudflareWAFRulesDataSourceID and i suspect you'll need to programmatically loop through the resource rules and confirm the key is present in the state following this change.

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.

we need to adjust the test case for this because as it stands, it is a duplicate of an existing test with a different name and doesn't cover the added fields being stored in state.

@JoseRoman JoseRoman marked this pull request as draft March 10, 2021 04:53
@jacobbednarz
Copy link
Member

closing in favour of #1079

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

Successfully merging this pull request may close these issues.

2 participants