-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
azurerm_web_application_firewall_policy: add http_listener_ids and path_based_rule_ids #10860
Conversation
@favoretti I'd like your input on whether I've placed the subresource ID parse/validate functions in the right files, as my first commit placed them inside files that are overwritten by go:generate. |
@sirlatrom I need to take a deeper look at this, but can't you just use the generators to generate these ID functions, rather than creating them by hand? |
@favoretti I'd love to! How do I get started? |
@sirlatrom Just add them here and go generate will generate them for you. |
azurerm/internal/services/network/web_application_firewall_policy_resource.go
Outdated
Show resolved
Hide resolved
…th_based_rule_ids Signed-off-by: Sune Keller <[email protected]>
…arate files Signed-off-by: Sune Keller <[email protected]>
…urces Signed-off-by: Sune Keller <[email protected]>
Signed-off-by: Sune Keller <[email protected]>
…e_ids Signed-off-by: Sune Keller <[email protected]>
Signed-off-by: Sune Keller <[email protected]>
Rebased on origin/master. |
@katbyte Is there any chance this PR could get on the next milestone? |
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.
Thanks for the pr @sirlatrom - aside from one comment i've left inline this is looking good!
if err := d.Set("managed_rules", flattenWebApplicationFirewallPolicyManagedRulesDefinition(webApplicationFirewallPolicyPropertiesFormat.ManagedRules)); err != nil { | ||
return fmt.Errorf("Error setting `managed_rules`: %+v", err) |
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.
Is there a reason we are removing managed rules from here?
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.
It's a good question, and the answer is above, as the lines were duplicated by mistake in my original contribution of managed_rules
:
1b5a0b3#diff-25ef614a1fbe2de62ec91d10ddde72c778c5876a77dc676908b065a8268846c5R344-R349
So it should be safe 😅
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.
🤦♀️ - that they are and i'membarrassedd that i missed that
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.
🤦♀️ - that they are and i'membarrassedd that i missed that
Don't be! I bet you have a lot on your plate with all the things happening in this provider. Thanks for the review! 😊
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 👍
This has been released in version 2.54.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.54.0"
}
# ... other configuration ... |
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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Fixes #10859.
Not sure about the syntax in the acceptance test when referencing the path based rule IDs.