d/aws_wafregional_web_acl: Add WAFRegional Web ACL lookup datasource #9321

robh007 · 2019-07-12T11:21:09Z

AWS WAFRegional Web ACL lookup datasource

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Related #2654

Release note for CHANGELOG:

** New datasource ** `aws_wafregional_web_acl`

Output from acceptance testing:

$ make testacc ./aws TESTARGS='-run=TestAccDataSourceAwsWafRegionalWebAcl'
--- PASS: TestAccDataSourceAwsWafRegionalWebAcl_Basic (26.85s)

...

bflad

Hi @robh007 👋 Thank you so much for this contribution! Overall this is looking pretty good, just left a few items below to fix up so we can merge this. 👍 Please reach out if you have any questions or do not have time to implement these items.

bflad · 2019-07-17T14:21:45Z

aws/data_source_aws_waf_web_acl_test.go

+)
+
+func TestAccDataSourceAwsWafRegionalWebAcl_Basic(t *testing.T) {
+	name := "tf-acc-test"


Can you please randomize the naming to prevent issues where the test may have left existing resources? Please see also the Uses Randomized Infrastructure Naming item of the Contributing Guide Thanks!

Suggested change

name := "tf-acc-test"

name := acctest.RandomWithPrefix("tf-acc-test")

bflad · 2019-07-17T14:22:25Z

aws/data_source_aws_waf_web_acl.go

+				Type:     schema.TypeString,
+				Required: true,
+			},
+			"id": {


Can you please remove the id attribute from the schema? It is implicit with all Terraform resources. Thanks!

bflad · 2019-07-17T15:06:48Z

aws/data_source_aws_waf_web_acl.go

@@ -0,0 +1,64 @@
+package aws


Can you please name this file aws/data_source_aws_wafregional_web_acl.go to match the data source naming? Thanks!

bflad · 2019-07-17T15:06:59Z

aws/data_source_aws_waf_web_acl_test.go

@@ -0,0 +1,78 @@
+package aws


Can you please name this file aws/data_source_aws_wafregional_web_acl_test.go to match the data source naming? Thanks!

bflad · 2019-07-17T15:25:27Z

aws/data_source_aws_waf_web_acl_test.go

+	})
+}
+
+func testAccDataSourceAwsWafRegionalWebAclConfig_Name(name string) string {


Since we do not need to verify the rules within the Web ACL and they are optional in the resource, can you please remove the extra infrastructure from the test configuration? Thanks!

func testAccDataSourceAwsWafRegionalWebAclConfig_Name(name string) string { return fmt.Sprintf(` resource "aws_wafregional_web_acl" "web_acl" { name = %[1]q metric_name = "tfWebACL" default_action { type = "ALLOW" } } data "aws_wafregional_web_acl" "web_acl" { name = "${aws_wafregional_web_acl.web_acl.name}" } `, name) }

bflad · 2019-07-17T15:53:10Z

website/docs/d/wafregional_web_acl.html

@@ -0,0 +1,30 @@
+---


Can you please add .markdown to the end of the file name here? Otherwise the website page does not load properly. You can verify this with make website.

bflad · 2019-07-17T15:54:06Z

website/docs/d/wafregional_web_acl.html

+page_title: "AWS: aws_wafregional_web_acl"
+sidebar_current: "docs-aws-datasource-wafregional-web-acl"
+description: |-
+Retrieves a WAF Regional Web ACL id.


Can you please fix the YAML here? Otherwise the website page does not load properly. You can verify this with make website.

Suggested change

Retrieves a WAF Regional Web ACL id.

Retrieves a WAF Regional Web ACL id.

bflad · 2019-07-17T15:54:32Z

website/docs/d/wafregional_web_acl.html

+
+```hcl
+data "aws_wafregional_web_acl" "example" {
+name = "tfWAFRule"


Terraform formatting:

Suggested change

name = "tfWAFRule"

name = "tfWAFRule"

robh007 · 2019-07-17T17:13:27Z

Thanks for the review @bflad, I'll get these sorted.

robh007 · 2019-07-17T17:43:28Z

Changes as requested. @bflad I pushed 3 new data resources for WAF with this work. So I'll go back and check all of those inline with these comments.

Retested.
$ make testacc ./aws TESTARGS='-run=TestAccDataSourceAwsWafRegionalWebAcl'
PASS: TestAccDataSourceAwsWafRegionalWebAcl_Basic (16.50s)

ryndaniels

Thanks for making these changes, @robh007! 🚀

FYI, I changed the wording in the initial comment from "fixes" to "related" - since #2654 spans multiple data sources, we don't want this one PR to close that whole issue (which the fixes keyword does).

Output from acceptance testing:

[TestAccDataSourceAwsWafRegionalWebAcl_Basic] [Test Output]
=== RUN   TestAccDataSourceAwsWafRegionalWebAcl_Basic
=== PAUSE TestAccDataSourceAwsWafRegionalWebAcl_Basic
=== CONT  TestAccDataSourceAwsWafRegionalWebAcl_Basic
--- PASS: TestAccDataSourceAwsWafRegionalWebAcl_Basic (10.47s)
PASS

👍

ghost · 2019-11-02T15:20:58Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Rob H added 2 commits July 11, 2019 22:46

added data source for WAFRegional Web ACL

84cb117

Updated comment

3f27586

robh007 requested a review from a team July 12, 2019 11:21

bflad assigned ryndaniels Jul 12, 2019

bflad added the new-data-source Introduces a new data source. label Jul 12, 2019

bflad suggested changes Jul 17, 2019

View reviewed changes

Updated after initial Review

aa59c67

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Jul 17, 2019

This was referenced Jul 17, 2019

d/aws_waf_web_acl: Add WAF Web ACL lookup datasource #9320

Merged

d/aws_waf_rule: Add WAF Rule lookup datasource #9318

Merged

d/aws_wafregional_rule: Add WAFRegional Rule lookup datasource #9319

Merged

bflad approved these changes Jul 18, 2019

View reviewed changes

ryndaniels approved these changes Jul 18, 2019

View reviewed changes

ryndaniels added this to the v2.20.0 milestone Jul 18, 2019

ryndaniels merged commit 54136ee into hashicorp:master Jul 18, 2019

bflad added a commit that referenced this pull request Jul 18, 2019

Update CHANGELOG formatting for #9320 and #9321

6a4934a

ghost locked and limited conversation to collaborators Nov 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

d/aws_wafregional_web_acl: Add WAFRegional Web ACL lookup datasource #9321

d/aws_wafregional_web_acl: Add WAFRegional Web ACL lookup datasource #9321

robh007 commented Jul 12, 2019 •

edited by ryndaniels

Loading

bflad left a comment

bflad Jul 17, 2019

bflad Jul 17, 2019

bflad Jul 17, 2019

bflad Jul 17, 2019

bflad Jul 17, 2019

bflad Jul 17, 2019

bflad Jul 17, 2019

bflad Jul 17, 2019

robh007 commented Jul 17, 2019

robh007 commented Jul 17, 2019

ryndaniels left a comment

ghost commented Nov 2, 2019

	name := "tf-acc-test"
	name := acctest.RandomWithPrefix("tf-acc-test")

	Retrieves a WAF Regional Web ACL id.
	Retrieves a WAF Regional Web ACL id.

d/aws_wafregional_web_acl: Add WAFRegional Web ACL lookup datasource #9321

d/aws_wafregional_web_acl: Add WAFRegional Web ACL lookup datasource #9321

Conversation

robh007 commented Jul 12, 2019 • edited by ryndaniels Loading

Community Note

bflad left a 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

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

robh007 commented Jul 17, 2019

robh007 commented Jul 17, 2019

ryndaniels left a comment

Choose a reason for hiding this comment

ghost commented Nov 2, 2019

robh007 commented Jul 12, 2019 •

edited by ryndaniels

Loading