Skip to content

Fix issues noticed during the refactor #56

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

Merged
merged 6 commits into from
May 12, 2025

Conversation

vozhyk-
Copy link
Collaborator

@vozhyk- vozhyk- commented May 12, 2025

Fix issues noticed during #47 and #53 that were dependent on other things being fixed.

  • meraki_organization: organizations_appliance_third_party_vpn_peers: Add missing fields - regenerate fields
    • public_hostname. Note: Meraki API returns an error if both public_hostname and public_ip are specified.
    • ipsec_policies_preset. Note: Meraki API ignores ipsec_policies if ipsec_policies_preset is specified. Idempotency breaks in that case.
  • meraki_organization: organizations_vpn_firewall_rules: Add missing syslog_default_rule field
  • meraki_appliance: networks_appliance_vpn_site_to_site_vpn: Add missing subnet_nat_is_allowed field - regenerate fields
  • meraki_appliance: networks_appliance_vlans_settings: Create the resource if no VLANs are configured.
    • Refactor code #47 (the part done manually) changed this to create the resource only if at least one VLAN is specified. Since the only value the resource sets is length(vlans) > 0, it's not possible to have it set to false after that change. It was originally created if domains.organizations are specified, which was always, since networks are under organizations.
    • Change this to create the resource if network.appliance is specified instead.

@vozhyk- vozhyk- requested review from jon-humphries and danischm May 12, 2025 11:57
@@ -519,7 +519,7 @@ locals {
key = format("%s/%s/%s", domain.name, organization.name, network.name)
network_id = meraki_network.organizations_networks[format("%s/%s/%s", domain.name, organization.name, network.name)].id
vlans_enabled = try(length(network.appliance.vlans) > 0, false)
} if try(length(network.appliance.vlans) > 0, false)
} if try(network.appliance, null) != null
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jon-humphries under what condition should the networks_appliance_vlans_settings resource be created?

Originally, it was always created; then #47 changed it to check network.appliance.vlans (i.e. to create a resource only if a VLAN is configured, i.e. only with vlans_enabled = true.

Copy link
Collaborator

@jon-humphries jon-humphries left a comment

Choose a reason for hiding this comment

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

Tested against internal pipeline job/pr-1575/2/console

Reviewed code changes, logic is good.

@jon-humphries jon-humphries merged commit f86b92f into main May 12, 2025
1 check passed
@vozhyk- vozhyk- deleted the more-fixes-after-refactor branch May 13, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants