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 Module Issue]: [Multiple modules] Parameters are missing intellisense and validation which should be provided via decorators and types #3641

Open
1 task done
StephenWeatherford opened this issue Oct 25, 2024 · 8 comments
Labels
Class: Resource Module 📦 This is a resource module Language: Bicep 💪 This is related to the Bicep IaC language Needs: Core Team 🧞 This item needs the AVM Core Team to review it Status: Long Term ⏳ We will do it, but will take a longer amount of time due to complexity/priorities Status: Looking For Assistance 🦆 This item is looking for anyone to help develop the code and submit a PR for resolution Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: Bug 🐛 Something isn't working

Comments

@StephenWeatherford
Copy link
Contributor

StephenWeatherford commented Oct 25, 2024

Check for previous/existing GitHub issues

  • I have checked for previous/existing GitHub issues

Issue Type?

Bug

Module Name

Diverse

(Optional) Module Version

This applies to all modules, but key-vault is an example

Description

Here's an example:

module modKeyvault1 'br/public:avm/res/key-vault/vault:0.9.0' = {
  name: 'keyvault1'
  params: {
    createMode:    <<<<  TRY INTELLISENSE HERE
    softDeleteRetentionInDays: 1         <<<< INVALID VALUE
    name: uniqueString(resourceGroup().id, 'keyvault demo')
  }
}
  1. Try placing the cursor after "createMode: "
    !) no intellisense
    EXPECTED: Should provide "default" and "recover" as possible values. Instead, I had to go to the help page (or deploy to find out the problem).
    FIX FOR YOUR MODULE: Use the following code for the param:
@description('Optional. The vault\'s create mode to indicate whether the vault need to be recovered or not. - recover or default.')
param createMode 'default' | 'recover' = 'default'
  1. The above value of softDeleteRetentionInDays is invalid, yet no validation is provided until I deploy
    EXPECTED: Should notify me in editor of incorrect value
    FIX FOR YOUR MODULE:
@description('Optional. softDelete data retention days. It accepts >=7 and <=90.')
@minValue(7)
@maxValue(90)
param softDeleteRetentionInDays int = 90

I'm sure there are plenty other examples. This should be standard expected practice, thanks.

(Optional) Correlation Id

No response

@StephenWeatherford StephenWeatherford added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels Oct 25, 2024
@github-project-automation github-project-automation bot moved this to Needs: Triage in AVM - Module Issues Oct 25, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Type: Bug 🐛 Something isn't working label Oct 25, 2024
@avm-team-linter avm-team-linter bot added the Class: Pattern Module 📦 This is a pattern module label Oct 25, 2024
Copy link

@StephenWeatherford, thanks for submitting this issue for the modulemodKeyvault1'br/public:avm/res/key-vault/vault:0.9.0'={ module!

Important

The module does not exist yet, we look into it. Please file a new module proposal under AVM Module proposal.

@StephenWeatherford
Copy link
Contributor Author

StephenWeatherford commented Oct 25, 2024

NOTE: I'm marking this as "bug" not "feature request" but this is standard expected behavior and IMHO it's a bug in today's environment to not have intellisense and validation for params.

Part of this is related to #1642.

@StephenWeatherford StephenWeatherford changed the title [AVM Module Issue]: Why aren't you using decorators and types to disallow invalid values and provide intellisense for parameters? [AVM Module Issue]: [Multiple modules] Parameters are missing intellisense and validation which should be provided via decorators and types Oct 25, 2024
@AlexanderSehr
Copy link
Contributor

Hey @StephenWeatherford,
101% agree. We did define a requirement for 1.0 that UDT's / intellisense must be implemented for all parameters, but rely on the module owners to implement it (for the most part). As of today we do not enforce it yet though (presumably we could introduce a test to do that, especially when it comes to objects & array of objects. AllowedSets is another example.
I guess one of the hopes was that we could eventually dynamically import types from the bicep-types repository (such a feature was presented quite some time ago), but to my knowledge it's still experimental. If it still is by the time we want to go 1.0, we'd go ahead and enforce the implementation of custom UDTs etc. across the board.

In any case thanks for raising this and yes,it should be linked to #1642, with the added case of the allowedSets etc. decorators.

cc: @eriqua, @ChrisSidebotham, @jtracey93

@AlexanderSehr AlexanderSehr added Status: Long Term ⏳ We will do it, but will take a longer amount of time due to complexity/priorities Class: Resource Module 📦 This is a resource module Status: Looking For Assistance 🦆 This item is looking for anyone to help develop the code and submit a PR for resolution Language: Bicep 💪 This is related to the Bicep IaC language Needs: Core Team 🧞 This item needs the AVM Core Team to review it and removed Needs: Triage 🔍 Maintainers need to triage still Class: Pattern Module 📦 This is a pattern module labels Oct 26, 2024
@eriqua eriqua pinned this issue Oct 29, 2024
@eriqua
Copy link
Contributor

eriqua commented Oct 29, 2024

Thanks @StephenWeatherford for raising this issue. I'm also pinning this one to further highlight it.

Echoing what @AlexanderSehr already pointed out, we expect this to be a long term one since we'll rely on module owners' work to complete it across the library.

In addition, as discussed in #1642, we were looking forward to 2 experimental features to go GA: compile-time imports and resourcederivedtypes.

Since the former got GA, as a first step, we're currently working on aligning AVM common types to be imported from the recently released avm/utl/types/avm-common-types module. That would align UDT for the shared interfaces, which are anyway already covered by UDT today for the most part.

For the remaining complex types such as objects and array of objects, the resourcederivedtypes GA feature would definitely help accelerating the process.

On the core team side, we are planning to provide specs and guidelines to achieve this, in addition to static validation tests, most likely initially implemented with a soft failure.
You can also reference this AVM issue aiming to cover most of the above aspects Azure/Azure-Verified-Modules#1559

@StephenWeatherford
Copy link
Contributor Author

Thanks, I'm glad it's on your radar!

@StephenWeatherford
Copy link
Contributor Author

@eriqua @AlexanderSehr FYI, the latest version of Bicep has new functionalities "extract parameter" and "extract type" which can make it much easier to take a resource and pull out parameters, which user-defined types and description filled out:

Image
=>
Image

@StephenWeatherford
Copy link
Contributor Author

It also supports resource-derived types if you have them enabled.

@AlexanderSehr
Copy link
Contributor

Thanks for the update @StephenWeatherford, I don't know if that's the blue background, but it looks very neat 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Class: Resource Module 📦 This is a resource module Language: Bicep 💪 This is related to the Bicep IaC language Needs: Core Team 🧞 This item needs the AVM Core Team to review it Status: Long Term ⏳ We will do it, but will take a longer amount of time due to complexity/priorities Status: Looking For Assistance 🦆 This item is looking for anyone to help develop the code and submit a PR for resolution Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: Bug 🐛 Something isn't working
Projects
Status: Needs: Triage
Development

No branches or pull requests

3 participants