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

Extendable Param Files feature #13900

Merged
merged 57 commits into from
Jul 8, 2024

Conversation

polatengin
Copy link
Member

@polatengin polatengin commented Apr 18, 2024

With this PR

  • we introduce a new keyword extends
  • using none is allowed

extends keyword make bicepparam file inherits from another bicepparam

using none is valid only on shared bicepparam files and it turns off the parameter validation process on the file

How does it work

Files:

// shared.bicepparam
using none
param location = 'westus'
param name = 'default_name'
// main.bicepparam
using './main.bicep'
extends 'shared.bicepparam'
param vm_sku = 'Standard_D4_v4'

Compiled output:

{
  "location": "westus",
  "name": "default_name",
  "vm_sku": "Standard_D4_v4"
}

Fixes #12019

Contributing a feature

  • I have opened a new issue for the proposal, or commented on an existing one, and ensured that the Bicep maintainers are good with the design of the feature being implemented
  • I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged
  • I have appropriate test coverage of my new feature
Microsoft Reviewers: Open in CodeFlow

@polatengin polatengin self-assigned this Apr 18, 2024
@polatengin polatengin linked an issue Apr 18, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Apr 18, 2024

Test this change out locally with the following install scripts (Action run 9843636081)

VSCode
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 9843636081
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 9843636081"
Azure CLI
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 9843636081
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 9843636081"

Copy link
Contributor

github-actions bot commented Apr 18, 2024

Test Results

    66 files   -     33      66 suites   - 33   23m 48s ⏱️ - 7m 57s
10 897 tests  -      6  10 895 ✅  -      7  1 💤 ±0  1 ❌ +1 
25 690 runs   - 12 799  25 686 ✅  - 12 800  2 💤  - 1  2 ❌ +2 

For more details on these failures, see this check.

Results for commit 2b30da2. ± Comparison against base commit a8eff9c.

♻️ This comment has been updated with latest results.

@ngetchell-pi
Copy link

What happens when main.bicepparam contains duplicate params as the shared param file?

@ChristopherGLewis
Copy link
Contributor

A couple of comments, some which should be addressed now and something in the future.

Understanding the override characteristics would be important -

  1. does the "primary" value override an extend or does the extend override the primary?
  2. Does a command line parameter override merged parameters?

Could we possibly get a decorator such as @MergeCharacteristcs with values:

  1. NeverOverride - this parameter will never be overridden with either an extend or CLI param
  2. Override - this parameter will be overridden by the extend (useful for arrays/objects)
  3. MergeObject - union arrays/objects rather than overriding. Allows us to define tags at a hierarchy level and build up tags via multiple extends

Can we support chained extends?
Given the tree:

C:.
+---dev
|   |   env.bicepparam
|   |
|   +---eastus
|   |       region.bicepparam
|   |
|   \---westus
|           region.bicepparam
|
\---prod
    |   env.bicepparam
    |
    +---eastus
    |       region.bicepparam
    |
    \---westus
            region.bicepparam

It would be great if our main.bicepparam would extend region.bicepparam and region extent env.bicepparm. That would give us a nice changed hierarchy.

Finally, can the extend use paths that are dynamic?

Given the above hierarchy, when I deploy an object using a main.bicepparam, I'd like to have the main have the following

main.bicepparam

using './main.bicep'
extends '../${readEnvironmentVariable('env')}/${readEnvironmentVariable('region')}/region.bicepparam'
param vm_sku = 'Standard_D4_v4'

region.bicepparam

using null
extends '../${readEnvironmentVariable('env')}/env.bicepparam'
param region = 'eastus'

env.bicepparam

using null
param env = 'prod'

@slavizh
Copy link
Contributor

slavizh commented May 3, 2024

I find the proposed design very limited. As far as I understood from the meeting the following things are not possible:

  • Intelisense for shared bicep parameters files
  • multiple extends statements
  • ability to nest shared bicep parameters files
  • ability to merge (union) parameters of type object and array. Merge tags scenarios
  • support for variables

I think better approach would be to have something like:

// shared.bicepparam
using 'shared'
using 'main1.bicep'
using 'main2.bicep'

In this scenario you are clearly defining that this is shared bicep parameters file. Also additionally you provide two (or more if you want, these are optional statements) bicep files to serve as intelisense for the shared file. In this scenario when you define that this is shared bicep parameters files any errors related to required parameters are turned off. In VSC you can still see from intelisense if specific parameter is required or not. You can also see the description for those parameters. In case main1.bicep and main2.bicep contains the same parameter let's say location and that contains two different descriptions upon hovering the parameter when added you can see the description for both. In summary you still get intelisense and other information like description, allowed values, etc, but without the errors you would get in regular bicep parameters files.

With that said I would like to see support for the rest of the missing features as well:

  • multiple extends statements
  • ability to nest shared bicep parameters files
  • ability to merge (union) parameters of type object and array. Merge tags scenarios
  • support for variables

Would also be nice that if you can declare which parameters from extends statement you would like to add instead of adding everything. Similar to how it is in import statements.
With the support for variables would be good to see their value when you use them on other parameters when you hover over the variable.

It would really make sense if all these features are released at once otherwise it is best to be preview feature until every feature is added.

@mmassey1993
Copy link

mmassey1993 commented May 3, 2024

I dont like the idea of typing "using null" as it seems a little clunky. I would rather it be some other form of MS decorator or keyword such as "using sharedbicepparam" which can imply that the bicepparam file can be extended to other files.

One other point with this, if this gets implemented, could this lead to a future development whereby we can use the "extends" command direct from a main.bicep file rather than a bicepparam file? This could lead to the possibility of having one global variables file whereby individual main files could 'import' selected variables from the sharedbicepparam file when needed. This could also be through the form of a function, like loadJsonContext(), but maybe something like loadBicepparamFile() ?

@mennolaan
Copy link

This pr doesn't explain nor solve how we can decouple the bicepparam files between modules and main bicep files. The biggest problem right now is you need to define all the parameters in the main bicep files. I have an extended explanation here on why this is an issue and how i think this could be tackled: #12200

A main bicep file should be rather clean of everything. If we have a look at where Microsoft is heading with the bicep verified modules, i believe Microsoft needs an implementation where they can host modules with their own bicep param files as a blueprint. Only the last part is now solved with the extend, where you can override the definition of the bicep files.

Copy link
Contributor

github-actions bot commented Jun 9, 2024

Dotnet Test Results

    72 files   -     36      72 suites   - 36   23m 12s ⏱️ - 9m 50s
10 949 tests  -      3  10 949 ✅  -      3  0 💤 ±0  0 ❌ ±0 
25 794 runs   - 12 842  25 794 ✅  - 12 842  0 💤 ±0  0 ❌ ±0 

Results for commit 0038c1f. ± Comparison against base commit 9864b14.

♻️ This comment has been updated with latest results.

@polatengin polatengin changed the title Modular Parameters feature Extendable Param Files feature Jun 27, 2024
@polatengin polatengin enabled auto-merge (squash) July 2, 2024 16:13
@slavizh
Copy link
Contributor

slavizh commented Jul 5, 2024

I have watched the last meeting but did not see any of the feedback given before implemented. In fact it seems that everything was the same and the only small change is that using is having none for value instead of null. That is only cosmetic change and it is irrelevant for the functionality. Kind of disappointed due to that.

@polatengin polatengin merged commit c6267d6 into main Jul 8, 2024
44 checks passed
@polatengin polatengin deleted the enpolat/12019-design-proposal-modular-parameters branch July 8, 2024 16:46
@m-soltani
Copy link

@polatengin : Please note that using parameters of type Object in shared.bicepparam file is currently throwing error in main param files. I think it is easy for you to reproduce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Design Proposal] Extendable Param Files feature