feat: Expand usage of UDT for the AVNM Bicep Module - avm/res/network/network-manager

[ahmadabdalla]

Description

Introducing User Defined Types for the Azure Virtual Network Manager (AVNM) Bicep module. Additionally, expanded on the test cases for the maxium to increase test coverage for UDT validations.

Pipeline Reference

Pipeline

Type of Change

• Update to CI Environment or utlities (Non-module effecting changes)
• Azure Verified Module updates:
  • Bugfix containing backwards compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
    • Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
    • The bug was found by the module author, and no one has opened an issue to report it yet.
  • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
  • Breaking changes and I have bumped the MAJOR version in version.json.
  • Update to documentation

Checklist

• I'm sure there are no other open Pull Requests for the same update/change
• I have run Set-AVMModule locally to generate the supporting module files.
• My corresponding pipelines / checks run clean and green without any errors or warnings

[AlexanderSehr]

There is actually a new spec waiting in a PR that asks primitive types to be declared as such (e.g. use int instead if string for numbers, or boolean instead of string for true/false).
This would mean, this parameter should be changed to isGlobal: bool and the place where it is used needs to (optionally) cast the value to a string. I've seen implicit casts in the pasts, so we may not need it.

Same comment for the other string-boolean parameters

[ahmadabdalla]

Interesting. I would avoid hiding such things and align to the API as much as possible. That way its a standard consumption method from an API and IaC perspective and less abstraction code to maintain. But if that's the path that AVM is going to take then I will wait for that PR to be merged. Would be great to know if that's something that will be as a recommendation for the module owner or something that is a must have.

[AlexanderSehr]

I'd argue it's probably a 'Should' that targets improved usability of the resource, which in many instances is just plain bad (case an point CMK or boolean strings 😄 )

[ahmadabdalla]

Hey @AlexanderSehr I have done some testing on the casting approach.

Here is an example PR: comparing the current UDT implementation as per this PR with an approach for casting.

It definetly took a toll on testing and validation a lot more than it was before. And seems more code to maintain for an owner. What are your thoughts on this? How has this been tested with other modules at a big scale?

I have some questions around the default behaviour for a UDT param of type bool. Because I had to use casting, I now am enforced in my test files to explicitly pass the parameter either true false compared to the past. Is there a specific pattern I should follow here as per AVM standards?

See above picture as an example. If I do not provide these values, my deployments now fail :/

[AlexanderSehr]

Hey @ahmadabdalla,
maybe we can find a slot some time to actually have a chat about the subject, as it's just easier to talk about than write 😄
Anyhow, the modules I've seen implement this on scale did usually not introduce much added code. Usually it was not much more than introducing e.g. a string(parameter) or '${parameter}' where a parameter was used an a string was expected, while in others it made the module actually easier as conditions didn't need to check for explicit string values in conditions like if(parameter == 'true'). But naturally, the argument you made (i.e., deviating from the API) was exactlythe discussion to have. In like 'is it worth it'. At least for now we opted for 'yes' on the backdrop that we want to improve/easy a deployments usuability when we can.

Regarding the default behavior - if a parameter is optional, you can still just make it nullable. Also booleans. Does this answer the question (not sure I fully understood it 😄)? Something else that speaks for a chat 😉

[AlexanderSehr]

Should ideally be networkGroupResourceId

Please note, if changed to e.g. networkGroupResourceId, also the location where it is used must be updated wth a map function to map it to the expected networkGroupId`

[AlexanderSehr]

Also mind you, as you implemented the UDT in mutliple files, it would also need to be fixed in multiple files.

Looking forward to the time when types can simply be imported via a types.bicep (ref)

[AlexanderSehr]

Same comment as before

Suggested change

	param sourcePortRanges sourcePortRangesType?
	param sourcePortRanges string[]?

[AlexanderSehr]

Just a suggestion (so feel free to just resolve), but you could actually leave the else case out when referencing the child module.

Example

• If in the child module param destinationPortRanges has a default in the child, and this line here is just destinationPortRanges: rule.?destinationPortRanges, then it will automatically pick the default in the child
• If in the child module param destinationPortRanges has no default but is nullable like in your case, then no value is passed through (which I guess also is fine)