-
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
Firewall rule related resources change block type from Set
to List
#15008
Firewall rule related resources change block type from Set
to List
#15008
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.
Hi @magodo - Could you add multiple value and update test coverage that shows the schema items changed here being updated with multiple values? The tests currently only have a single value in each case, so don't confirm the changes are correct?
Thanks!
@jackofallops I've made the single elements to multiples to cover all the changed properties. Please take another look, thanks! |
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.
Lots of test failures here:
------- Stdout: -------
=== RUN TestAccFirewallApplicationRuleCollection_basic
=== PAUSE TestAccFirewallApplicationRuleCollection_basic
=== CONT TestAccFirewallApplicationRuleCollection_basic
testing_new.go:70: Error running post-test destroy, there may be dangling resources: exit status 1
Error: deleting Azure Firewall Firewall: (Azure Firewall Name "acctestfirewall220128025321388480" / Resource Group "acctestRG-fw-220128025321388480") : network.AzureFirewallsClient#Delete: Failure sending request: StatusCode=415 -- Original Error: Code="UnsupportedMediaType" Message="The content media type '<null>' is not supported. Only 'application/json' is supported."
--- FAIL: TestAccFirewallApplicationRuleCollection_basic (541.65s)
FAIL
@katbyte That's strange, since the test result of 11e1e8c only has two failed cases, then I submit two new commits to fix them, then it failed a lot... Update... I've run some of the tests locally (by merging the main branch) and it all works well. Also the main branch for the firewall RP starts to fail since Jan.27. After checking the changes, I didn't find anything suspicious (that explains why my local run succeeds). Also, my local run has the same environment settings as the TC (same location, and even the |
Might be worth mentioning, I've noticed since about Jan 31st that our firewall deployments we are suddenly getting errors from our deployments with configuration changes to the Azure Firewall and associated rules despite no obvious change to our deployments that are causing this. We have a case open with microsoft for the issue as I suspect it may be an upstream issue by them. I don't know if it's related to the errors experienced here but I figured I'd mention it. |
Hi @magodo, We are also looking forward to this fix as it would resolve our blocking issue. When do we expect this PR to be completed? |
@katbyte Can we merge this? The test failures are not related to this issue, and should have been fixed in another PR (merged). |
Which PR is this, just for reference? |
So from what I gathered, this PR would stop the deletion of rule collection blocks when creating new rules in azure firewall and azure firewall policy? Will the terraform plan also show each individual rule to be added (ie. if adding 3 new rules, terraform plan will say 'resources: 3 added')? |
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 functionality has been released in v3.0.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
There are typically two reasons to use the set type:
It turns out (#11181, #10083), the first benefit is not more valuable than a readable plan. After contacting with the Azure firewall PM that he confirms the order of rules together with the other compound properties are reserved in turns of API, this PR changes the type of those blocks from
TypeSet
toTypeList
.Fixes #11181, fixes #10083.