palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402

aochsner · 2024-12-31T21:30:13Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Palo Alto changed the plan id and the one hard coded in the provider is no longer valid. This keeps the old one for backwards compatibility. Would like to backport this to v3.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests. - I'm unable to run this as my company doesn't let me.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

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_virtual_network_panorama - support for marketplace_details and plan_data properties [palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402]
azurerm_palo_alto_next_generation_firewall_virtual_network_local_rulestack - support for marketplace_details and plan_data properties [palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402]
azurerm_palo_alto_next_generation_firewall_virtual_hub_panorama - support for marketplace_details and plan_data properties [palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402]
azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack - support for marketplace_details and plan_data properties [palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #28267

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

neil-yechenwei · 2025-01-08T02:01:43Z

internal/services/paloalto/next_generation_firewall_vhub_local_rulestack_resource.go

@@ -73,6 +75,10 @@ func (r NextGenerationFirewallVHubLocalRuleStackResource) Arguments() map[string

 		"destination_nat": schema.DestinationNATSchema(),

+		"plan_data": schema.PlanDataSchema(),
+
+		"marketplace_details": schema.MarketplaceDetailsSchema(),


Suggest to arrange the newly added fields in alphabetical order. So please swap the position of "plan_data" and "marketplace_details". Other places also need to be updated. Thanks.

neil-yechenwei · 2025-01-08T03:22:16Z

internal/services/paloalto/schema/plan_data.go

+				"plan_id": {
+					Type:         pluginsdk.TypeString,
+					Optional:     true,
+					ForceNew:     true,


Could you double confirm that this property is updateable once it's created since service team mentioned that planId should be updateable without the need to delete and recreate the resource? Please also check if "billing_cycle" and "usage_type" is updateable. Thanks.

neil-yechenwei · 2025-01-08T03:26:31Z

internal/services/paloalto/schema/plan_data.go

+					Type:         pluginsdk.TypeString,
+					Optional:     true,
+					ForceNew:     true,
+					Default:      "panw-cloud-ngfw-payg",


Service team mentioned that the default value is changed to "panw-cngfw-payg". Could you double check it? Thanks.

neil-yechenwei · 2025-01-08T03:27:54Z

internal/services/paloalto/schema/plan_data.go

+					Optional:     true,
+					ForceNew:     true,
+					Default:      "panw-cloud-ngfw-payg",
+					ValidateFunc: validation.StringIsNotEmpty,


Suggest adding validation per property description.

neil-yechenwei · 2025-01-08T03:32:06Z

internal/services/paloalto/schema/marketplace_details.go

@@ -0,0 +1,72 @@
+// Copyright (c) HashiCorp, Inc.


Suggest adding these new properties to resource docs. Thanks.

neil-yechenwei · 2025-01-08T03:37:10Z

internal/services/paloalto/next_generation_firewall_vhub_local_rulestack_resource.go

@@ -29,10 +29,12 @@ type NextGenerationFirewallVHubLocalRuleStackModel struct {
 	ResourceGroupName string `tfschema:"resource_group_name"`


Suggest triggering the related test cases for the changed resources on your local environment to ensure there is no regression issue. Please also paste the test result in this PR. Thanks.

neil-yechenwei · 2025-01-08T03:49:53Z

internal/services/paloalto/next_generation_firewall_vhub_local_rulestack_resource.go

@@ -29,10 +29,12 @@ type NextGenerationFirewallVHubLocalRuleStackModel struct {
 	ResourceGroupName string `tfschema:"resource_group_name"`
 	RuleStackId       string `tfschema:"rulestack_id"`


Suggest add/update test case for newly added properties. Thanks.

neil-yechenwei · 2025-01-08T03:57:21Z

internal/services/paloalto/schema/plan_data.go

+					Type:         pluginsdk.TypeString,
+					Optional:     true,
+					ForceNew:     true,
+					Default:      firewalls.UsageTypePAYG,


Could you double check if it has the default value since Swagger indicates it's not a required property? Thanks.

neil-yechenwei · 2025-01-08T03:57:48Z

internal/services/paloalto/schema/plan_data.go

+			result.PlanId = p.PlanId
+		}
+
+		if p.UsageType != "" {


Suggest use "pointer.To(firewalls.UsageType(p.UsageType))".

neil-yechenwei · 2025-01-13T06:47:23Z

@aochsner , thanks for raising this PR. I've taken a look through and left some comments inline. If we can fix those up, this should be good to go. May I ask if there is ETA for the fix? Your PR is also very useful for other customers, which is why both Microsoft and Hashicorp are paying attention to it.

neil-yechenwei · 2025-01-16T01:22:20Z

@aochsner , Thanks for raising this issue. If we have not received your comments or fixes by this Friday, we will take over the fix. Thank you for your understanding.

aochsner · 2025-01-16T02:01:32Z

Hi sorry for not responding sooner. I will do my best to review the comments. Thank you for the feedback. I have no problem letting yall take over for it. We have been able to work around our immediate blockers via the azapi provider. But I did a best effort to raise the issue for a proper fix while it's all fresh in my mind

neil-yechenwei · 2025-01-16T04:31:37Z

@aochsner , I noticed that you also made "marketplaceDetails.publisherId," "planData.BillingCycle," and "planData.usageType" configurable parameters. However, the service team only expects planId and offerId to be configurable. Therefore, I will submit a new PR where only planId and offerId will be made configurable parameters. Would that be okay? If you agree, please close this PR. Thanks.

aochsner · 2025-01-16T05:07:52Z

Yep I agree. Happy to close

aochsner added 2 commits December 31, 2024 13:32

make Marketplace Details & Plan Data configurable for Palo Alto NGFW

69d96e6

Merge branch 'main' into feature/palo-alto-parameterize-offer-plan

0595273

aochsner requested a review from a team as a code owner December 31, 2024 21:30

github-actions bot added bug enhancement service/paloalto size/XL waiting-response labels Dec 31, 2024

fix unit test

b3de900

github-actions bot added waiting-response and removed waiting-response labels Jan 2, 2025

fix unit test

29a7e07

github-actions bot removed the waiting-response label Jan 2, 2025

neil-yechenwei reviewed Jan 8, 2025

View reviewed changes

github-actions bot added the waiting-response label Jan 8, 2025

alpha ordering

1b4a70e

github-actions bot added waiting-response and removed waiting-response labels Jan 8, 2025

neil-yechenwei reviewed Jan 8, 2025

View reviewed changes

github-actions bot removed the waiting-response label Jan 16, 2025

aochsner closed this Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402

palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402

aochsner commented Dec 31, 2024 •

edited

Loading

neil-yechenwei Jan 8, 2025 •

edited

Loading

neil-yechenwei Jan 8, 2025 •

edited

Loading

neil-yechenwei Jan 8, 2025 •

edited

Loading

neil-yechenwei Jan 8, 2025 •

edited

Loading

neil-yechenwei Jan 8, 2025

neil-yechenwei Jan 8, 2025

neil-yechenwei Jan 8, 2025

neil-yechenwei Jan 8, 2025

neil-yechenwei Jan 8, 2025

neil-yechenwei commented Jan 13, 2025 •

edited

Loading

neil-yechenwei commented Jan 16, 2025 •

edited

Loading

aochsner commented Jan 16, 2025

neil-yechenwei commented Jan 16, 2025 •

edited

Loading

aochsner commented Jan 16, 2025

		@@ -29,10 +29,12 @@ type NextGenerationFirewallVHubLocalRuleStackModel struct {
		ResourceGroupName string `tfschema:"resource_group_name"`

		@@ -29,10 +29,12 @@ type NextGenerationFirewallVHubLocalRuleStackModel struct {
		ResourceGroupName string `tfschema:"resource_group_name"`
		RuleStackId string `tfschema:"rulestack_id"`

palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402

palo_alto_next_generation_firewall make Marketplace Details and Plan Data configurable #28402

Conversation

aochsner commented Dec 31, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

neil-yechenwei Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

neil-yechenwei Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

neil-yechenwei Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

neil-yechenwei Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

neil-yechenwei Jan 8, 2025

Choose a reason for hiding this comment

neil-yechenwei Jan 8, 2025

Choose a reason for hiding this comment

neil-yechenwei Jan 8, 2025

Choose a reason for hiding this comment

neil-yechenwei Jan 8, 2025

Choose a reason for hiding this comment

neil-yechenwei Jan 8, 2025

Choose a reason for hiding this comment

neil-yechenwei commented Jan 13, 2025 • edited Loading

neil-yechenwei commented Jan 16, 2025 • edited Loading

aochsner commented Jan 16, 2025

neil-yechenwei commented Jan 16, 2025 • edited Loading

aochsner commented Jan 16, 2025

aochsner commented Dec 31, 2024 •

edited

Loading

neil-yechenwei Jan 8, 2025 •

edited

Loading

neil-yechenwei Jan 8, 2025 •

edited

Loading

neil-yechenwei Jan 8, 2025 •

edited

Loading

neil-yechenwei Jan 8, 2025 •

edited

Loading

neil-yechenwei commented Jan 13, 2025 •

edited

Loading

neil-yechenwei commented Jan 16, 2025 •

edited

Loading

neil-yechenwei commented Jan 16, 2025 •

edited

Loading