-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
New Resource: aws_wafregional_geo_match_set #3915
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few minor items below for consideration 👍
Delete: resourceAwsWafRegionalGeoMatchSetDelete, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"name": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: Extraneous &schema.Schema
here and below
|
||
resp, err := conn.GetGeoMatchSet(params) | ||
if err != nil { | ||
if isAWSErr(err, "WAFNonexistentItemException", "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar note here as other PR about upstream SDK issue.
Also, minor duplicate WAF WAF
in the error below. (Aside: I kind of wonder if we should have some sort of FriendlyName
attribute on resources)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of wonder if we should have some sort of FriendlyName attribute on resources
It sounds like the time is coming to reopen discussion around hashicorp/terraform#13582 😄
{ | ||
Config: testAccAWSWafRegionalGeoMatchSetConfigChangeName(geoMatchSetNewName), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckAWSWafRegionalGeoMatchSetExists("aws_wafregional_geo_match_set.test", &after), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect an acceptance test here that compares &before
to &after
(via ID or however necessary)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small suggestion of moving some common funcs to waf_helpers.go
} | ||
|
||
d.Set("name", resp.GeoMatchSet.Name) | ||
d.Set("geo_match_constraint", flattenWafGeoMatchConstraint(resp.GeoMatchSet.GeoMatchConstraints)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider moving flattening func to waf_helpers.go
req := &waf.UpdateGeoMatchSetInput{ | ||
ChangeToken: token, | ||
GeoMatchSetId: aws.String(id), | ||
Updates: diffWafGeoMatchSetConstraints(oldConstraints, newConstraints), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider moving diff func to waf_helpers.go
915b514
to
4c6be4f
Compare
4c6be4f
to
48ba898
Compare
48ba898
to
f65ae94
Compare
Addressed all your feedback and reran acceptance tests:
|
This has been released in version 1.13.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Test results