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

new resource: azurerm_firewall_policy #7390

Merged
merged 10 commits into from
Sep 22, 2020

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jun 18, 2020

This implements resource/data source azurerm_firewall_policy

Test Result

💤 😡 2 via 🦉 v1.14.4 make testacc TEST=./azurerm/internal/services/network/tests TESTARGS='-run "TestAccAzureRMFirewallPolicy_|TestAccDataSourceFirewallPoli
cy_basic"'
==> Checking that code complies with gofmt requirements...                
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/network/tests -v -run "TestAccAzureRMFirewallPolicy_|TestAccDataSourceFirewallPolicy_basic" -timeout 180m -ldflag
s="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccDataSourceFirewallPolicy_basic                        
=== PAUSE TestAccDataSourceFirewallPolicy_basic                                                                                                               
=== RUN   TestAccAzureRMFirewallPolicy_basic                                                                                                                  
=== PAUSE TestAccAzureRMFirewallPolicy_basic                                  
=== RUN   TestAccAzureRMFirewallPolicy_complete                                                                                                               
=== PAUSE TestAccAzureRMFirewallPolicy_complete                      
=== RUN   TestAccAzureRMFirewallPolicy_update                                                                                                                 
=== PAUSE TestAccAzureRMFirewallPolicy_update                                                                                                                 
=== RUN   TestAccAzureRMFirewallPolicy_requiresImport                                                                                                         
=== PAUSE TestAccAzureRMFirewallPolicy_requiresImport
=== RUN   TestAccAzureRMFirewallPolicy_inherit                         
=== PAUSE TestAccAzureRMFirewallPolicy_inherit                         
=== CONT  TestAccDataSourceFirewallPolicy_basic
=== CONT  TestAccAzureRMFirewallPolicy_requiresImport
=== CONT  TestAccAzureRMFirewallPolicy_complete                 
=== CONT  TestAccAzureRMFirewallPolicy_update                           
=== CONT  TestAccAzureRMFirewallPolicy_basic                         
=== CONT  TestAccAzureRMFirewallPolicy_inherit                                                                                                                
--- PASS: TestAccAzureRMFirewallPolicy_complete (102.93s)                                                                                                     
--- PASS: TestAccAzureRMFirewallPolicy_basic (105.53s)
--- PASS: TestAccDataSourceFirewallPolicy_basic (106.07s)
--- PASS: TestAccAzureRMFirewallPolicy_requiresImport (113.00s)
--- PASS: TestAccAzureRMFirewallPolicy_inherit (114.86s)         
--- PASS: TestAccAzureRMFirewallPolicy_update (184.15s)
PASS                                                                           
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/tests       184.206s

fixes #7319
fixes #7368
fixes #8363

@magodo
Copy link
Collaborator Author

magodo commented Jun 18, 2020

There might be a duplication effort on this resource in #7368

Copy link
Collaborator

@WodansSon WodansSon left a 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! 🚀

@magodo
Copy link
Collaborator Author

magodo commented Jul 2, 2020

@WodansSon Thank you for your review comments! I have made some changes accordingly while leaving some comments above for some concerns otherwise.

@tombuildsstuff tombuildsstuff added the sdk/requires-newer-api-version This requires upgrading the version of the API being used label Jul 2, 2020
@magodo
Copy link
Collaborator Author

magodo commented Jul 3, 2020

@WodansSon I have updated the code per your first two comments, please take a look. Thank you!

@ghost ghost added size/XXL and removed size/XL labels Jul 8, 2020
@magodo
Copy link
Collaborator Author

magodo commented Jul 8, 2020

@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.

Copy link
Collaborator

@WodansSon WodansSon left a 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! 🚀

Copy link
Collaborator

@katbyte katbyte left a 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

@magodo
Copy link
Collaborator Author

magodo commented Jul 14, 2020

@katbyte Thank you for your comments!
I have made the changes accordingly, please take a look!

@magodo magodo requested a review from katbyte July 14, 2020 09:36
@katbyte katbyte added waiting-response and removed sdk/requires-newer-api-version This requires upgrading the version of the API being used labels Jul 14, 2020
@tombuildsstuff
Copy link
Contributor

I think this is blocked until the Networking API issues have been resolved?

@jturver1
Copy link

@tombuildsstuff - Could you please confirm which networking API issues are blocking this?
vWAN API is changing all the time, but Azure Firewall Manager policies seems static since going GA from what I can see, and this PR relates to the Firewall Manager config only?
Many thanks for all the great work!

@ghost ghost removed the waiting-response label Jul 17, 2020
@magodo
Copy link
Collaborator Author

magodo commented Jul 18, 2020

@jturver1 The blocking issue is #7691, and will be fixed once next Go SDK release.

@magodo
Copy link
Collaborator Author

magodo commented Aug 28, 2020

@tombuildsstuff can we go on reviewing this PR and get it merged?

@magodo magodo mentioned this pull request Sep 7, 2020
@WodansSon WodansSon modified the milestones: Blocked, v2.28.0 Sep 10, 2020
@jackofallops jackofallops modified the milestones: v2.28.0, v2.29.0 Sep 17, 2020
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @magodo, LGTM 👍

@katbyte katbyte merged commit d94d9d8 into hashicorp:master Sep 22, 2020
katbyte added a commit that referenced this pull request Sep 22, 2020
@ghost
Copy link

ghost commented Sep 24, 2020

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 ...

@ghost
Copy link

ghost commented Oct 22, 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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for DNS settings Support for Azure Firewall Manager Support for Azure Firewall Manager resources
6 participants