search

Azure / Azure-Verified-Modules

Azure Verified Modules (AVM) is an initiative to consolidate and set the standards for what a good Infrastructure-as-Code module looks like. Modules will then align to these standards, across languages (Bicep, Terraform etc.) and will then be classified as AVMs and available from their respective language specific registries.
https://aka.ms/AVM
MIT License
275 stars 61 forks source link

[AVM Compliance Testing]: Allow Underscores in Microsoft.AzureStackHCI/clusters/deploymentSettings #1029

Open mbrat2005 opened 2 weeks ago

mbrat2005 commented 2 weeks ago

Check for previous/existing GitHub issues

Description

I'm getting the following validation error due to properties in user-defined types which have underscores. The property names match the REST API names: https://github.com/Azure/azure-rest-api-specs/blob/432838208e52bdf6267b8a566331ba10893c2076/specification/azurestackhci/resource-manager/Microsoft.AzureStackHCI/stable/2024-01-01/deploymentSettings.json#L776

Per the conversation in the AVM Bicep Discussion Team's channel, please add an exception for this type to allow the underscores.

image

AlexanderSehr commented 2 weeks ago

@eriqua & @jtracey93, given the dicussion in the channel, I guess we'll go with not changing the spec, but only allowing selected exceptions.

The only alternative I'd see would be the introduction of a User-Defined-Type that does not use the _ in its names and maps them to the RP-expected property names. But it would come with the downside that the owner would need to maintain the UDT as opposed to relying on the built-in Bicep-types (though to be fair, as long as type-imports are not available, there is currently no bicep-types support anyways).

For the route of exceptions, I'd suggest to check during the test if the RP is exactly the one in question, and than for it, exclude only the specific parameters - as opposed to excluding the entire module. If we do that, we need to presumably fetch the relevant property names to exclude from the Azure Template Reference or API Specs. The former does not contain the latest template version though.

eriqua commented 2 weeks ago

@AlexanderSehr these above are 2 alternatives, right? With UDT route we won't need to rework the test, which is tempting, but it would also be the most opinionated approach. Would you anyway see the route of exceptions still the most convenient, thinking forward to the bicep types import? If not for that, I wouldn't be against the UDT mapping idea. We are already mapping names for a few cases, although this scenario is slightly different.

Fetching names from the API specs sounds the most dynamic, but I'm afraid it may get a bit cumbersome.

AlexanderSehr commented 2 weeks ago

@AlexanderSehr these above are 2 alternatives, right? With UDT route we won't need to rework the test, which is tempting, but it would also be the most opinionated approach. Would you anyway see the route of exceptions still the most convenient, thinking forward to the bicep types import? If not for that, I wouldn't be against the UDT mapping idea. We are already mapping names for a few cases, although this scenario is slightly different.

Fetching names from the API specs sounds the most dynamic, but I'm afraid it may get a bit cumbersome.

Ah nono, I didn't mean fetching as this could break relatively easily. Just to find all the parameter names there to put them into the code 😉

I think there was already a decision for the exceptions so let's do that I guess. But I guess if it becomes a pattern we should either escelate or enforce the 'no underscore rule' across the board.

That being said, the next challenge will be to find time to implement this exception soon to unblock @mbrat2005. I'll add a 'Help wanted' label.

mbrat2005 commented 2 days ago

Proposed workaround: https://github.com/Azure/bicep-registry-modules/pull/2580