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]: Strong types for parameters #1642

Open
1 task done
weikanglim opened this issue Apr 10, 2024 · 12 comments
Open
1 task done

[AVM Module Issue]: Strong types for parameters #1642

weikanglim opened this issue Apr 10, 2024 · 12 comments
Assignees
Labels
Class: Resource Module 📦 This is a resource module Needs: Attention 👋 Reply has been added to issue, maintainer to review Needs: External Changes ⚒️ When an issue/PR requires changes that are outside of the control of the module. e.g. to an RP. Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: Feature Request ➕ New feature or request

Comments

@weikanglim
Copy link

Check for previous/existing GitHub issues

  • I have checked for previous/existing GitHub issues

Issue Type?

Feature Request

Module Name

avm/res/app/container-app

(Optional) Module Version

No response

Description

Note: I'm using container-app as an example, but the feedback applies generally to all object types surfaced in AVM.

When creating a container app, it is common to supply the container definition (containers) and a list of secrets. Currently, the module doesn't provide strongly typed parameters to do so. They are simply array, or object.

@description('Required. List of container definitions for the Container App.')
param containers array

@description('Optional. List of specialized containers that run before app containers.')
param initContainersTemplate array = []

@description('Optional. The secrets of the Container App.')
@secure()
param secrets object = {}

This unfortunately means that I cannot effectively use the module without also looking at the actual implementation detail for Microsoft.App/containerApps.

In general, I expect that using an AVM module should be as easy as authoring the underlying RP resource. It would be great if all the documentation already present on the RP swagger would just show up when we're doing a simple passthrough in the module. This may require upstream Bicep support to scale, but I'm opening the issue here to start the discussion.

(Optional) Correlation Id

No response

@weikanglim weikanglim added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels Apr 10, 2024
@github-project-automation github-project-automation bot moved this to Needs: Triage in AVM - Module Issues Apr 10, 2024
@github-actions github-actions bot added Needs: Attention 👋 Reply has been added to issue, maintainer to review Class: Resource Module 📦 This is a resource module labels Apr 10, 2024
Copy link
Contributor

@weikanglim, thanks for submitting this issue for the avm/res/app/container-app module!

A member of the @azure/avm-res-app-containerapp-module-owners-bicep or @azure/avm-res-app-containerapp-module-contributors-bicep team will review it soon!

@microsoft-github-policy-service microsoft-github-policy-service bot added the Status: Response Overdue 🚩 When an issue/PR has not been responded to for X amount of days label Apr 16, 2024
@oZakari
Copy link
Contributor

oZakari commented Apr 17, 2024

Hi @weikanglim, I agree completely and will work on getting this in.

Adding @AlexanderSehr as it sounds like this maybe needs to be scaled to additional modules as well.

@oZakari oZakari removed the Status: Response Overdue 🚩 When an issue/PR has not been responded to for X amount of days label Apr 17, 2024
@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented Apr 17, 2024

Hey @weikanglim,
thanks for raising the issue and @oZakari for chiming in.

The ask is absolutely valid and yes it all module owners ideally do that. I think the only arrays & objects that currenty have this consistently are the extension resources (e.g., role assignments, private endpoints, etc.).

Up until now we had contributions here and there from community or the owners, adding UDTs to individual modules - but not on scale.

Personally, there are 2 reasons I've not been running around adding / asking to add user defined types to modules:

  1. The import of types from a types file is still an 'experimental' feature and I'd strongly advocate for using it once available to not redefine the same objects over and over - especially for modules with child resources
  2. The PG demoed already a feature that would allow us to import type information (unless customized) from the bicep types. So, for a property we could simply define the type by importing it via a Bicep-native function, saving us a lot of work

Either feature seems to be released soon, so instead of investing time now in manually creating every type, it might be wise to wait for them to be available and not 'undo' the work.

These are anyways only my subjective thoughts on the matter. When I'm using AVM modules and there is such a case (object without a type), I do usually one of 2 things:

  • Check the Usage Examples / directly the e2e tests folder for examples on how the object looks like
  • Or, if it's not a customized object, actually use the Azure Docs that describe the same

Neither is as good as UDTs, of course, but at least helped me to bridge the gap for the time being.

I hope this was somewhat insightful. Please let me know what you think.

@alex-frankel
Copy link

+1 on Alex's comments. Once those two features are non-experimental, can we/should we introduce CI validation to enforce that simple object and array types are no longer allowed?

@oZakari oZakari added the Needs: External Changes ⚒️ When an issue/PR requires changes that are outside of the control of the module. e.g. to an RP. label Apr 17, 2024
@oZakari
Copy link
Contributor

oZakari commented Apr 17, 2024

Thanks for the additional insight @AlexanderSehr. I will hold off on adding the UDTs for the time being until the experimental features are GA. I've added the External Changes label but let me know if there is anything else I need to do as the module owner.

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented Apr 17, 2024

+1 on Alex's comments. Once those two features are non-experimental, can we/should we introduce CI validation to enforce that simple object and array types are no longer allowed?

Hey @alex-frankel, thanks for adding your thoughts and yes.
Maybe in an incremental approach to not outright blocks contributions (as a full enforcement will block both internal & external contributions until the tests pass again), but I'm sure we can figure something out that gets this done fast while keeping the barrier low. 💪

cc: @eriqua, @ChrisSidebotham, @jtracey93

@eriqua
Copy link
Contributor

eriqua commented Apr 17, 2024

+1 on Alex's comments. Once those two features are non-experimental, can we/should we introduce CI validation to enforce that simple object and array types are no longer allowed?

Hey @alex-frankel, thanks for adding your thoughts and yes. Maybe in an incremental approach to not outright blocks contributions (as a full enforcement will block both internal & external contributions until the tests pass again), but I'm sure we can figure something out that gets this done fast while keeping the barrier low. 💪

cc: @eriqua, @ChrisSidebotham, @jtracey93

I agree with the general request and remember that has been the plan especially for child modules since UDT has been released as experimental back in the CARML days.

I'd also vouch for a SHOULD over a MUST, i.e., an incremental approach and a static validation soft failure once above experimental features go GA.

I'm thinking about edge cases such as Microsoft.Web sites/config or the siteConfig object. I'm sure others will pop up.

@alex-frankel
Copy link

alex-frankel commented Apr 22, 2024

Up to you all on SHOULD vs MUST. I'd love to see a transition to MUST at some point, but certainly SHOULD is great progress.

By the way, compile-time imports is now GA. So we are only waiting on resourceDerivedTypes. @jeskew / @stephaniezyen - do we have an ETA on this feature leaving experimental?

@microsoft-github-policy-service microsoft-github-policy-service bot added the Status: Response Overdue 🚩 When an issue/PR has not been responded to for X amount of days label Apr 26, 2024
@AlexanderSehr AlexanderSehr removed the Status: Response Overdue 🚩 When an issue/PR has not been responded to for X amount of days label Apr 26, 2024
@jamesharling
Copy link

jamesharling commented Jun 6, 2024

Eagerly awaiting this feature - use of UDTs coupled with RDTs would make for a really rich development experience. I want to migrate to AVM ASAP but the overhead of having to look at implementation details to get my array/object contents correct is just too big and time-consuming at the moment.

@r4hulp
Copy link

r4hulp commented Aug 15, 2024

I also observed this in other module vpn-site and thought of initiating a discussion but I am glad that it is already being discussed. This is a major improvement that is awaiting bicep to roll out the experimental feature into main stream.

@peterbud
Copy link
Contributor

I have seen import feature already being used in this repo (see sql server). Does this mean that we are OK to use that strong types and import features? I'd love to do that TBH

@AlexanderSehr
Copy link
Contributor

I have seen import feature already being used in this repo (see sql server). Does this mean that we are OK to use that strong types and import features? I'd love to do that TBH

Hey @peterbud,
the import from other Bicep files absolutely. We're actually soon releasing the AVM-Common-Types module that will also reduce the code in each module using AVM's interfaces for the extension resources: #3397

Also, on the note of UDTs, we'll also introduce a spec asking module owners to add the @export() flag to all their UDTs that may have relevance for module consumers so they can import them from the Public Registry (e.g., the KeyVault module has a UDT for Keys that somebody using that module may want to re-use in their pattern). The same should be done for child-modules so that when those are eventually published too, all custom AVM UDTs would be available for the community to use.

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 Needs: Attention 👋 Reply has been added to issue, maintainer to review Needs: External Changes ⚒️ When an issue/PR requires changes that are outside of the control of the module. e.g. to an RP. Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: Feature Request ➕ New feature or request
Projects
Status: Needs: Triage
Development

No branches or pull requests

8 participants