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 RP Palo Alto #22700

Merged
merged 74 commits into from Aug 10, 2023
Merged

New RP Palo Alto #22700

merged 74 commits into from Aug 10, 2023

Conversation

jackofallops
Copy link
Member

@jackofallops jackofallops commented Jul 26, 2023

Adds new RP Palo Alto, consisting of

  • New Resource: azurerm_palo_alto_local_rulestack
  • New Resource: azurerm_palo_alto_local_rulestack_certificate
  • New Resource: azurerm_palo_alto_local_rulestack_fqdn_list
  • New Resource: azurerm_palo_alto_local_rulestack_outbound_trust_certificate_association
  • New Resource: azurerm_palo_alto_local_rulestack_outbound_untrust_certificate_association
  • New Resource: azurerm_palo_alto_local_rulestack_prefix_list
  • New Resource: azurerm_palo_alto_local_rulestack_rule
  • New Resource: azurerm_palo_alto_virtual_network_appliance
  • New Resource: azurerm_palo_alto_next_generation_firewall_virtual_hub_local_rulestack
  • New Resource: azurerm_palo_alto_next_generation_firewall_virtual_hub_panorama
  • New Resource: azurerm_palo_alto_next_generation_firewall_virtual_network_local_rulestack
  • New Resource: azurerm_palo_alto_next_generation_firewall_virtual_network_panorama
  • New Data Source: azurerm_palo_alto_local_rulestack

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @jackofallops

Thanks for this PR - I've taken a look through and left some comments inline, but on the whole this is looking pretty good.

In general most of the comments fall under:

  1. Schema consistency (for example removing None as a value in favour of omitting the value
  2. Field naming consistency (e.g. using public_ip_address_id rather than public_ip_id etc)
  3. Resource naming consistency (e.g. virtual_hub rather than vhub)
  4. Extended timeouts for the resources (as discussed offline) - these are currently 2h for testing purposes, but can likely be reduced
  5. Working through the remaining TODOs
  6. Validating the API Response has a model (if model := resp.Model; model != nil { .. })
  7. Missing documentation for the new data source/resources

However on the whole this is looking pretty good, so nice work - if we can fix those comments up then this should otherwise be good to go 👍

Thanks!

internal/services/network/client/client.go Outdated Show resolved Hide resolved
internal/services/paloalto/client/client.go Outdated Show resolved Hide resolved
internal/services/paloalto/local_rule_stack_data_source.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/network_profile.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/network_profile.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/network_profile.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/network_profile.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/network_profile.go Outdated Show resolved Hide resolved
Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @jackofallops

Thanks for this PR - I've taken a look through and left some comments inline, but on the whole this is looking pretty good.

In general most of the comments fall under:

  1. Schema consistency (for example removing None as a value in favour of omitting the value
  2. Field naming consistency (e.g. using public_ip_address_id rather than public_ip_id etc)
  3. Resource naming consistency (e.g. virtual_hub rather than vhub)
  4. Extended timeouts for the resources (as discussed offline) - these are currently 2h for testing purposes, but can likely be reduced
  5. Working through the remaining TODOs
  6. Validating the API Response has a model (if model := resp.Model; model != nil { .. })
  7. Missing documentation for the new data source/resources

However on the whole this is looking pretty good, so nice work - if we can fix those comments up then this should otherwise be good to go 👍

Thanks!

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

A few minor comments but 👍

.teamcity/components/settings.kt Outdated Show resolved Hide resolved
internal/services/paloalto/schema/destination.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/destination_nat.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/destination_nat.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/destination_nat.go Outdated Show resolved Hide resolved
internal/services/paloalto/schema/source.go Outdated Show resolved Hide resolved
@jackofallops jackofallops merged commit d5f65ba into main Aug 10, 2023
23 checks passed
@jackofallops jackofallops deleted the r/paloalto-firewall branch August 10, 2023 06:57
@github-actions github-actions bot added this to the v3.69.0 milestone Aug 10, 2023
jackofallops added a commit that referenced this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants