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 association type for aws_wafregional_web_acl rules #4978

Merged
merged 2 commits into from
Jun 25, 2018
Merged

add association type for aws_wafregional_web_acl rules #4978

merged 2 commits into from
Jun 25, 2018

Conversation

svenwltr
Copy link
Contributor

Fixes #4184

Changes proposed in this pull request:

  • add type field for aws_wafregional_web_acl, so it supports aws_wafregional_rate_based_rule resources

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSWafRegionalWebAcl_createRateBased'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSWafRegionalWebAcl_createRateBased -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSWafRegionalWebAcl_createRateBased
--- PASS: TestAccAWSWafRegionalWebAcl_createRateBased (20.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	(cached)
...

I would really like to add a check for the type attribute, but I don't know how to figure out the hash (xxx in the example below).

resource.TestCheckResourceAttr(
	"aws_wafregional_web_acl.waf_acl", "rule.xxx.type", "RATE_BASED"),

This commit adds rule type support so that Rate Limit rules
could be use along with REGULAR rules.

Closes #4079 #4174 #4052
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Jun 25, 2018
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/waf Issues and PRs that pertain to the waf service. labels Jun 25, 2018
@bflad
Copy link
Contributor

bflad commented Jun 25, 2018

Hi @svenwltr 👋 thanks for submitting this! We actually have an open pull request for this functionality that was just missing the testing that you have: #4307

Would you be able to instead branch off that existing work and apply your testing in a commit after those? We should be able to release this immediately afterwards, we just want to be sure the original author gets credit as well. Thanks so much!

@bflad bflad added this to the v1.25.0 milestone Jun 25, 2018
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Jun 25, 2018
@svenwltr
Copy link
Contributor Author

Uhm, okay. Done.

% make testacc TESTARGS='-run=TestAccAWSWafRegionalWebAcl_createRateBased'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSWafRegionalWebAcl_createRateBased -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSWafRegionalWebAcl_createRateBased
--- PASS: TestAccAWSWafRegionalWebAcl_createRateBased (26.02s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	26.037s

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

After updating computeWafRegionalWebAclRuleIndex to accept the rule type as well, this looks great! 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccAWSWafRegionalWebAcl_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSWafRegionalWebAcl_ -timeout 120m
=== RUN   TestAccAWSWafRegionalWebAcl_basic
--- PASS: TestAccAWSWafRegionalWebAcl_basic (19.00s)
=== RUN   TestAccAWSWafRegionalWebAcl_createRateBased
--- PASS: TestAccAWSWafRegionalWebAcl_createRateBased (19.31s)
=== RUN   TestAccAWSWafRegionalWebAcl_changeNameForceNew
--- PASS: TestAccAWSWafRegionalWebAcl_changeNameForceNew (32.45s)
=== RUN   TestAccAWSWafRegionalWebAcl_changeDefaultAction
--- PASS: TestAccAWSWafRegionalWebAcl_changeDefaultAction (33.24s)
=== RUN   TestAccAWSWafRegionalWebAcl_disappears
--- PASS: TestAccAWSWafRegionalWebAcl_disappears (17.45s)
=== RUN   TestAccAWSWafRegionalWebAcl_noRules
--- PASS: TestAccAWSWafRegionalWebAcl_noRules (13.67s)
=== RUN   TestAccAWSWafRegionalWebAcl_changeRules
--- PASS: TestAccAWSWafRegionalWebAcl_changeRules (30.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	165.650s

@omeid
Copy link
Contributor

omeid commented Jun 26, 2018

Thank you so much for working to land this. @svenwltr.

@svenwltr svenwltr deleted the fix-waf branch June 26, 2018 08:11
@bflad
Copy link
Contributor

bflad commented Jun 27, 2018

This has been released in version 1.25.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 4, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/waf Issues and PRs that pertain to the waf service. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot associate an aws_wafregional_rate_based_rule with an aws_wafregional_web_acl resouce
3 participants