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

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

Closed
1 task done
mbrat2005 opened this issue Jun 10, 2024 · 4 comments · Fixed by Azure/bicep-registry-modules#2580
Labels
Language: Bicep 💪 This is related to the Bicep IaC language Needs: Core Team 🧞 This item needs the AVM Core Team to review it Status: Help Wanted 🆘 Extra attention is needed Type: Question/Feedback 🙋 Further information is requested or just some feedback

Comments

@mbrat2005
Copy link

Check for previous/existing GitHub issues

  • I have checked 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

@mbrat2005 mbrat2005 added Needs: Triage 🔍 Maintainers need to triage still Type: Question/Feedback 🙋 Further information is requested or just some feedback labels Jun 10, 2024
@github-project-automation github-project-automation bot moved this to Needs: Triage in AVM - Issue Triage Jun 10, 2024
@AlexanderSehr
Copy link
Contributor

@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
Copy link
Contributor

eriqua commented Jun 10, 2024

@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
Copy link
Contributor

@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.

@AlexanderSehr AlexanderSehr added Status: Help Wanted 🆘 Extra attention is needed Needs: Core Team 🧞 This item needs the AVM Core Team to review it and removed Needs: Triage 🔍 Maintainers need to triage still labels Jun 10, 2024
@matebarabas matebarabas added the Language: Bicep 💪 This is related to the Bicep IaC language label Jun 12, 2024
@mbrat2005
Copy link
Author

Proposed workaround: Azure/bicep-registry-modules#2580

mbrat2005 added a commit to Azure/bicep-registry-modules that referenced this issue Jul 3, 2024
## Description

Fixes Azure/Azure-Verified-Modules#1029

## Pipeline Reference


![image](https://github.com/Azure/bicep-registry-modules/assets/25390936/c3bf13f1-d70d-4bb2-9228-d12bad7ec99a)

## Type of Change

<!-- Use the checkboxes [x] on the options that are relevant. -->

- [x] Update to CI Environment or utilities (Non-module affecting
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

<!-- Please keep up to date with the contribution guide at
https://aka.ms/avm/contribute/bicep -->
@github-project-automation github-project-automation bot moved this from Needs: Triage to Done in AVM - Issue Triage Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language: Bicep 💪 This is related to the Bicep IaC language Needs: Core Team 🧞 This item needs the AVM Core Team to review it Status: Help Wanted 🆘 Extra attention is needed Type: Question/Feedback 🙋 Further information is requested or just some feedback
Projects
Development

Successfully merging a pull request may close this issue.

4 participants