-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: azurerm_firewall_policy #7390
Conversation
There might be a duplication effort on this resource in #7368 |
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.
@magodo, thank you so much for this PR... this looks really good, but I've left a few really minor nit-picky comments, but other than that this LGTM! Nice job! 🚀
@WodansSon Thank you for your review comments! I have made some changes accordingly while leaving some comments above for some concerns otherwise. |
@WodansSon I have updated the code per your first two comments, please take a look. Thank you! |
6e3c06c
to
ee3c5c6
Compare
@WodansSon I have rebased the PR on top of current master branch, so that I can switch the API version to 2020-05-01 for this resource. I also added some new properties accordingly. |
a226825
to
ddd8c36
Compare
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.
@magodo, thanks for the change this LGTM now! Thanks again for the PR! 🚀
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 @magodo, i've left some comments and questions about schema naming but otherwise this looks pretty good
azurerm/internal/services/network/tests/firewall_policy_resource_test.go
Outdated
Show resolved
Hide resolved
@katbyte Thank you for your comments! |
I think this is blocked until the Networking API issues have been resolved? |
@tombuildsstuff - Could you please confirm which networking API issues are blocking this? |
@tombuildsstuff can we go on reviewing this PR and get it merged? |
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 @magodo, LGTM 👍
This has been released in version 2.29.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.29.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! |
This implements resource/data source
azurerm_firewall_policy
Test Result
fixes #7319
fixes #7368
fixes #8363