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

Added several user defined types, ability for custom resources names in vwanConnectivity and mgDiagSettings #656

Merged
merged 32 commits into from
Nov 20, 2023

Conversation

johnlokerse
Copy link
Contributor

@johnlokerse johnlokerse commented Oct 13, 2023

Overview/Summary

The changes in this PR allow the user to set custom names for Azure Firewall, Virtual WAN Hubs, ExpressRoute Gateway, and VPN Gateway. Also, for readability, I added the virtualWanOptionsType so the array of objects is easier to understand, read and work with.

Issue: #625

AB#30600

This PR fixes/adds/changes/removes

  1. Added virtualWanOptionsType
  2. Introduced optional parVpnGatewayCustomName, parExpressRouteGatewayCustomName, parAzFirewallCustomName, and parVirtualWanHubCustomName paramaters to specify custom resource names.
  3. Added custom name capability for mgDiagSettings.bicep
  4. Added custom name capability for Log Analytics Linked Service
  5. Added subnetOptionsType
  6. Added nonComplianceMessageType

Breaking Changes

Not breaking due to the conditional values for VWAN module changes as well as new parameters using previously hard coded value

Testing Evidence

Succeeded deployment without custom names (as is situation):

image

Succeeded deployment with custom names:

image

As part of this Pull Request I have

@jtracey93
Copy link
Collaborator

Thanks @johnlokerse,

dont forget to update the parameter input files also 👍

Copy link
Collaborator

@jtracey93 jtracey93 left a comment

Choose a reason for hiding this comment

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

@johnlokerse we've got some test failures, could you review and resolve?

Also could you review the other modules to ensure they all allow a custom name for resources, if desired and don't have any hardcoded patterns like this one did?

@johnlokerse
Copy link
Contributor Author

Added succeeded deployment screenshots

@johnlokerse
Copy link
Contributor Author

Thanks @johnlokerse,

dont forget to update the parameter input files also 👍

How do we want to present the parameter file? The new properties are fully optional so there is no need to add them to the parameter file. It is only required when you want to use a custom name.

@johnlokerse we've got some test failures, could you review and resolve?

Also could you review the other modules to ensure they all allow a custom name for resources, if desired and don't have any hardcoded patterns like this one did?

Will do! :)

@johnlokerse johnlokerse changed the title Added type virtualWanOptionsType and introduced the ability for custom resource names Added several user defined types, ability for custom resources names in vwanConnectivity and mgDiagSettings Oct 13, 2023
@johnlokerse johnlokerse requested a review from a team as a code owner November 6, 2023 12:37
@johnlokerse
Copy link
Contributor Author

Fixed the broken Action regarding listing the used resource types. The underlying ARM template is different when using User-Defined Types. The Action was looking for the resource type $_.Type but it cannot be found when using User-Defined Types, because the type is nested deeper inside a PSObject.

I have tested this locally and got it working like it was before:

image

In this test I had two JSON files (hubNetworking.json and policyAssignedManagementGroup.json) with User-Defined Types and one JSON file (publicIp.json) without types.

Can you rerun the Action for me @jtracey93 / @oZakari?

@johnlokerse
Copy link
Contributor Author

@johnlokerse
Copy link
Contributor Author

johnlokerse commented Nov 6, 2023

FYI. I am testing something so that I do not have to use the contains method. Much cleaner code and is easier to read. Will commit the change tonight after successful test deployments!

@picccard
Copy link
Contributor

picccard commented Nov 6, 2023

Is there any need for parUseCustomNamingScheme?
Since the custom-names are optional, could you just use the custom-name if any is provided or a default value if omitted?

resource resAzureFirewall 'Microsoft.Network/azureFirewalls@2023-02-01' = [for (hub, i) in parVirtualWanHubs: if ((parVirtualHubEnabled) && (hub.parAzFirewallEnabled)) {
  name: hub.?parAzFirewallName ?? '${parAzFirewallName}-${hub.parHubLocation}'
  // this returns hub.parAzFirewallName if not null, else return default name
  ...
}

@johnlokerse
Copy link
Contributor Author

johnlokerse commented Nov 6, 2023

Is there any need for parUseCustomNamingScheme? Since the custom-names are optional, could you just use the custom-name if any is provided or a default value if omitted?

resource resAzureFirewall 'Microsoft.Network/azureFirewalls@2023-02-01' = [for (hub, i) in parVirtualWanHubs: if ((parVirtualHubEnabled) && (hub.parAzFirewallEnabled)) {
  name: hub.?parAzFirewallName ?? '${parAzFirewallName}-${hub.parHubLocation}'
  // this returns hub.parAzFirewallName if not null, else return default name
  ...
}

Yes, that was my change to reduce complexity/add readability :-) Thanks for the suggestion @picccard!

Tested the solution through deployments:

Successful deployment without 'custom names':
custom_base

Successful deployment with 'custom names':
custom_v2

@oZakari oZakari self-requested a review November 7, 2023 02:42
@oZakari
Copy link
Contributor

oZakari commented Nov 7, 2023

Thank you much @johnlokerse! I will plan on going through this PR later this week.

@oZakari oZakari self-assigned this Nov 7, 2023
@oZakari oZakari dismissed jtracey93’s stale review November 17, 2023 02:52

Already has been resolved by John

@oZakari oZakari closed this Nov 17, 2023
@oZakari oZakari reopened this Nov 17, 2023
@oZakari
Copy link
Contributor

oZakari commented Nov 17, 2023

Hey @johnlokerse, for the VWAN user defined type, I removed the parUseCustomNamingScheme switch references as it looks like you had removed that from the conditions in a previous commit. I also adjusted the properties for the user defined type in that module as well. Does what I changed it to make sense to you?

Everything else looks good for the PR, let me know your thoughts. Thanks again for all of your work with this :)

@johnlokerse
Copy link
Contributor Author

johnlokerse commented Nov 20, 2023

Looks good to me! Thanks for updating and let me know if I can help with something else 👍🏼💪🏼

@oZakari
Copy link
Contributor

oZakari commented Nov 20, 2023

/azp run validateazcloud

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@oZakari oZakari merged commit 2ee5422 into Azure:main Nov 20, 2023
10 checks passed
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.

4 participants