azurerm_palo_alto_next_generation_firewall_* - support for new properties marketplace_offer_id and plan_id
#28537
+379
−41
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…k - support for new properties marketplace_offer_id and plan_id
…port for new properties marketplace_offer_id and plan_id
…stack - support for new properties marketplace_offer_id and plan_id
… support for new properties marketplace_offer_id and plan_id
stephybun
requested changes
Jan 28, 2025
Comment on lines
+177
to
+195
| if !features.FivePointOhBeta() { | ||
| return fmt.Sprintf(` | ||
| %[1]s | ||
| resource "azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack" "import" { | ||
| name = azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack.test.name | ||
| resource_group_name = azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack.test.resource_group_name | ||
| rulestack_id = azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack.test.rulestack_id | ||
| plan_id = azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack.test.plan_id | ||
| network_profile { | ||
| virtual_hub_id = azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack.test.network_profile.0.virtual_hub_id | ||
| network_virtual_appliance_id = azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack.test.network_profile.0.network_virtual_appliance_id | ||
| public_ip_address_ids = azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack.test.network_profile.0.public_ip_address_ids | ||
| } | ||
| } | ||
| `, r.basic(data)) | ||
| } | ||
| return fmt.Sprintf(` |
There was a problem hiding this comment.
I'm not sure this is necessary since plan_id is an optional property, the existing config for the requires import test should still work?
Comment on lines
+133
to
+154
| if !features.FivePointOhBeta() { | ||
| return fmt.Sprintf(` | ||
| provider "azurerm" { | ||
| features {} | ||
| } | ||
| %[1]s | ||
| resource "azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack" "test" { | ||
| name = "acctest-ngfwvh-%[2]d" | ||
| resource_group_name = azurerm_resource_group.test.name | ||
| rulestack_id = azurerm_palo_alto_local_rulestack.test.id | ||
| plan_id = "panw-cngfw-payg" | ||
| network_profile { | ||
| virtual_hub_id = azurerm_virtual_hub.test.id | ||
| network_virtual_appliance_id = azurerm_palo_alto_virtual_network_appliance.test.id | ||
| public_ip_address_ids = [azurerm_public_ip.test.id] | ||
| } | ||
| } | ||
| `, r.template(data), data.RandomInteger) | ||
| } |
There was a problem hiding this comment.
The basic test config shouldn't include optional arguments. I think this can be removed since we've added plan_id to the complete test configuration
| resource_group_name = azurerm_resource_group.test.name | ||
| rulestack_id = azurerm_palo_alto_local_rulestack.test.id | ||
| marketplace_offer_id = "pan_swfw_cloud_ngfw" | ||
| plan_id = "panw-cngfw-payg" |
There was a problem hiding this comment.
Can this be panw-cloud-ngfw-payg so we can test the update path of this property?
Comment on lines
+113
to
+135
| if !features.FivePointOhBeta() { | ||
| return fmt.Sprintf(` | ||
| provider "azurerm" { | ||
| features {} | ||
| } | ||
|
|
||
| %[1]s | ||
|
|
||
| resource "azurerm_palo_alto_next_generation_firewall_virtual_hub_panorama" "test" { | ||
| name = "acctest-ngfwvh-%[2]d" | ||
| resource_group_name = azurerm_resource_group.test.name | ||
| location = azurerm_resource_group.test.location | ||
| panorama_base64_config = "%[3]s" | ||
| plan_id = "panw-cngfw-payg" | ||
|
|
||
| network_profile { | ||
| virtual_hub_id = azurerm_virtual_hub.test.id | ||
| network_virtual_appliance_id = azurerm_palo_alto_virtual_network_appliance.test.id | ||
| public_ip_address_ids = [azurerm_public_ip.test.id] | ||
| } | ||
| } | ||
| `, r.template(data), data.RandomInteger, os.Getenv("ARM_PALO_ALTO_PANORAMA_CONFIG")) | ||
| } |
There was a problem hiding this comment.
Same remark here, optional properties shouldn't be added to basic tests, I think this can be removed since it's been added to the complete config. This comment applies to all the other resource tests that have been modified in this PR.
Comment on lines
+151
to
+166
| ### `azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack` | ||
|
|
||
| * The `plan_id` property now defaults to `panw-cngfw-payg`. | ||
|
|
||
| ### `azurerm_palo_alto_next_generation_firewall_virtual_hub_panorama` | ||
|
|
||
| * The `plan_id` property now defaults to `panw-cngfw-payg`. | ||
|
|
||
| ### `azurerm_palo_alto_next_generation_firewall_virtual_network_local_rulestack` | ||
|
|
||
| * The `plan_id` property now defaults to `panw-cngfw-payg`. | ||
|
|
||
| ### `azurerm_palo_alto_next_generation_firewall_virtual_network_panorama` | ||
|
|
||
| * The `plan_id` property now defaults to `panw-cngfw-payg`. | ||
|
|
There was a problem hiding this comment.
The upgrade guide follows alphabetical ordering, these need to be moved up
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Community Note
Description
Recently, I noticed that the test cases related to "azurerm_palo_alto_next_generation_firewall_" have been consistently failing with below error on TeamCity Daily Run. The error message indicates that "panw-cloud-ngfw-payg" has been deprecated. So service API doesn't allow it while provisioning the resource. Additionally, the service team has recently reached out to us, requesting that we update the default value of planId and make both planId and offerId configurable parameters. The following linked GitHub issue was also created by the service team.
Error Message:
PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
TF 4.x:
TF 5.x:
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_palo_alto_next_generation_firewall_*- support for new propertiesmarketplace_offer_idandplan_idThis is a (please select all that apply):
Related Issue(s)
Fixes #28267
Fixes #28526
Note
If this PR changes meaningfully during the course of review please update the title and description as required.