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

[Design Proposal] Extendable Param Files feature #12019

Closed
polatengin opened this issue Oct 2, 2023 · 28 comments · Fixed by #13900
Closed

[Design Proposal] Extendable Param Files feature #12019

polatengin opened this issue Oct 2, 2023 · 28 comments · Fixed by #13900
Assignees
Labels

Comments

@polatengin
Copy link
Member

polatengin commented Oct 2, 2023

Extendable Param Files Feature Design Proposal

As a Bicep developer, I want to be able to extend bicepparam files from other bicepparam files, so that I can reuse parameters across multiple deployments.

In most deployment scenarios, there are parameters that are common across multiple deployments. For example, a location parameter is often used in multiple deployments. It would be useful to be able to define the location parameter in a single file, and reuse it in other .bicepparam files.

Parent GitHub Issue: Bicep Parameters Upcoming Features #10777

Goal is to be able to extend parameter files from others with an extends directive

Proposed Solution

Syntax

extends '<path_to_base_bicepparam_file>'

Scenario

Combine parameters from the main.bicepparam, and a separate .bicepparam file;

// base.bicepparam
using none

param foo = 'foo'
param bar = 'bar'

// main.bicepparam
using 'main.bicep'

extends 'base.bicepparam'

param foo = 'bar'
param baz = foo

// compiled.json
{
  foo: 'bar'
  bar: 'bar'
  baz: 'bar'
}

Because the foo parameter is defined in both the base.bicepparam and main.bicepparam files, the foo parameter in the main.bicepparam file will override the foo parameter in the base.bicepparam file.

extends statements can only be used in .bicepparam files.

using statements are required in .bicepparam files, so, in shared/base .bicepparam files, the using none statement is required.

@polatengin polatengin added the enhancement New feature or request label Oct 2, 2023
@polatengin polatengin self-assigned this Oct 2, 2023
@alex-frankel
Copy link
Collaborator

Will there by a specific syntax for importing all param values from a file?

@cwp-michaell
Copy link

I like these ideas
Perhaps something a little cleaner

param location = import(location, ./foo.bicepparam) // This imports a single value
import(./bar.bicepparam) // This imports all values

The env variable idea could be done by extending the current var usage to the above

var env = 'dev'

param location = import(location, ./foo${env}.bicepparam)
import(./bar${env}.bicepparam}) // This imports all values

I'm not sure the named import would be needed with the above approach as any imported param should just be able to be referenced from the .bicep file.

Alternatively perhaps an include in the .bicep file pointing to any required bicepparam files would be a better approach?

@tg123
Copy link

tg123 commented Oct 9, 2023

will this support shadow param?

for exmaple

default.bicep

param a int = 1
param b int = 2

eastus.bicep

import * from default.bicep

param a int = 3

eastus.json would be

{
a : 3
b : 2
}

@WhitWaldo
Copy link

I would like to see both a wildcard import to import all parameters from a given file in addition to selecting specific params.

How would name conflicts be handled between files? Personally, I'd prefer a preference towards specificity (e.g. import the specifically named import over a wildcard, but show a squiggly line and build warning indicating the conflict).

@glloyd2010f
Copy link

I would like to see both a wildcard import to import all parameters from a given file in addition to selecting specific params.

How would name conflicts be handled between files? Personally, I'd prefer a preference towards specificity (e.g. import the specifically named import over a wildcard, but show a squiggly line and build warning indicating the conflict).

Expanding on this, seen customers where they like the generate a parameter file for an entire environment which will support multiple deployments. Was wondering if we could then do selective filtering based on an initial import of parameters.

Rough code example of this idea

allParams = import { * } from './global/${readEnvironmentVariable('env')}.bicepparam
import { location, name } from allParams

@polatengin
Copy link
Member Author

Will there be a specific syntax for importing all param values from a file?

great question!

Importing everything from a separate file could be dangerous, since you have to provide only the parameters required in the deployment, and that separate file could be updated and have new parameters for totally different deployments.

This could make the deployments that have "everything" syntax fail.

We'd like to have a conservative approach here, to make sure if the deployment can be made with the current content of the files, it'll still be made even if you add new parameters into the other file.

But we're thinking to add some features into the VSCode Extension, so authors of bicepparam files will easily get the list of parameters that can be imported.

@polatengin
Copy link
Member Author

I like these ideas Perhaps something a little cleaner

param location = import(location, ./foo.bicepparam) // This imports a single value
import(./bar.bicepparam) // This imports all values

in different places in bicep, we already have import syntax. it'd be better to reuse that syntax, so developers (especially the ones who are learning bicep) don't have to learn and remember two similar import syntax.

@polatengin
Copy link
Member Author

will this support shadow param?

for exmaple

default.bicep

param a int = 1
param b int = 2

eastus.bicep

import * from default.bicep

param a int = 3

eastus.json would be

{
a : 3
b : 2
}

we intentionally won't support everything syntax, the main reason is, eastus.bicepparam in your example can get new parameter definitions over time, to support "other" deployments, but it could fail the deployments, that uses eastus.bicepparam

I also mentioned this issue here; #12019 (comment)

@polatengin
Copy link
Member Author

I would like to see both a wildcard import to import all parameters from a given file in addition to selecting specific params.

Wildcard imports could be dangerous and can make the deployments that have no change, fail. (as mentioned here; #12019 (comment))

I'd like to understand better, what's the use-case of a wildcard import, if there is an editor support and you can easily import required parameters?

I see wildcard import syntax is to make importing several parameters easily. is it still the case, if the editor helps you to import required parameters easily?

@polatengin
Copy link
Member Author

How would name conflicts be handled between files? Personally, I'd prefer a preference towards specificity (e.g. import the specifically named import over a wildcard, but show a squiggly line and build warning indicating the conflict).

bicep (vscode and vs extensions, language server, cli, etc.) parse bicep and bicepparam files not order-dependent ( at least, so far 😄 )

and we're not going to support wildcard import syntax.

so, developers should import parameters by their names, and if there is a conflict, developer should resolve the conflict.

alias syntax ( import { appName as name } from 'foo.bicepparam' ) can be used to resolve this kind of issues

@polatengin
Copy link
Member Author

Expanding on this, seen customers where they like the generate a parameter file for an entire environment which will support multiple deployments. Was wondering if we could then do selective filtering based on an initial import of parameters.

Rough code example of this idea

allParams = import { * } from './global/${readEnvironmentVariable('env')}.bicepparam
import { location, name } from allParams

I think it's a very interesting idea. Although how much I like the idea, I don't see better developer experience between these two code blocks;

allParams = import { * } from './global/${readEnvironmentVariable('env')}.bicepparam
import { location, name } from allParams
import { location, name } from './global/${readEnvironmentVariable('env')}.bicepparam

in fact, the first proposal seems more complex, and, longer to type 😄

two import syntax does two different things (import but don't add parameters to context, and import and add parameters to context) could be confusing, too.

@WhitWaldo
Copy link

WhitWaldo commented Oct 26, 2023

I'd like to understand better, what's the use-case of a wildcard import, if there is an editor support and you can easily import required parameters?

  1. Bicep is already subject to a wall of parameters with their comments and decorators, and large multi-line object declarations due to its aversion to commas. I'd really rather avoid further polluting the top of my file with additional lengthy imports that would effectively do the same thing as a wildcard import.

  2. User-defined data types support wildcard imports and a mix/match style. It doesn't feel like a great future developer experience to have different (but near-identical-looking) import syntaxes based on what's being imported.

More to the point, developers have been spoiled and are fairly used to having the option of wildcard imports. They're extremely common in other languages. I've got to imagine there'd be a sizable number of issues over time of new Bicep developers inquiring as to what they're doing wrong in that their wildcard import statements don't work for parameters, but they do for user-defined types.

  1. If I'm dropping all my parameters in some file to avoid some typing, I'd like to save myself additional time and just insert an import * from '../my.params at the top of all of my files. It seems like an unnecessarily ceremonial step to narrow the imports from all to a select few.

Even with some editor support, it's a poor experience in Resharper already when I drop in a wall of new types into a class and inevitably have to go one by one through the new types to have it locate and add the correct using statement for each. Given the opportunity, I'd rather avoid a repeat of that poor UX here as well.

we intentionally won't support everything syntax, the main reason is, eastus.bicepparam in your example can get new parameter definitions over time, to support "other" deployments, but it could fail the deployments, that uses eastus.bicepparam

I've been an advocate for constant-folding + reliable what-if local simulated deployments and this just feels like another excellent use case. Even today absent parameter files, a change to a parameter can break a deployment and there's no way to know about it absent repeated real deployments to Azure in temporary resource groups (which costs real money).

I personally wouldn't rely on re-using parameter files in the short term because the tooling simply isn't here to fully validate complex deployments with anywhere near the accuracy of a real deployment. I mean, to your point, the parameters you started with in eastus.bicepparam could easily change over time simply because someone forgot they were already used elsewhere... but wildcard or specific import statements don't help there either. Your objection is little different from user types - those might change over time and have non-default parameters added which would break existing uses of them.. and as I mentioned above, they support wildcard imports.

@dstarkowski
Copy link

We'd like to have a conservative approach here

That seems more opinionated than conservative. Why go out of the way to deliberately take away the choice from developers and make the language less consistent?

Without wildcard, adding a shared parameter results in modifying all parameter files that use it. Whether I have editor support or not, every file needs to be modified, every file is changed in git, and every file needs to be reviewed. And probably doing the change without editor help will be quicker.

@brwilkinson
Copy link
Collaborator

Just catching up on the community calls. Are you still looking for feedback?

Is this work already underway?

@evkaky
Copy link

evkaky commented Mar 14, 2024

Would love to see this feature. There is so much copy and paste params across my .bicepparam files

@mrkesu
Copy link

mrkesu commented Apr 15, 2024

Is this far from finishing? This would finally let me move away from having to use CUE lang for parameters...

@polatengin polatengin linked a pull request Apr 18, 2024 that will close this issue
3 tasks
@alex-frankel
Copy link
Collaborator

I believe we are getting close to getting this done and released as an experimental feature. @stephaniezyen / @polatengin can provide a more precise ETA.

@polatengin
Copy link
Member Author

we have a draft PR is up (#13900)

our plan is to make it to the next release, behind a feature flag.

@ChristopherGLewis
Copy link
Contributor

ChristopherGLewis commented May 2, 2024

Some comments here: #13900 (comment)

@ryancolley
Copy link

Are we any closer to this being merged in yet?
I have been eagerly watching the releases and its not appeared yet :(

@stephaniezyen
Copy link
Contributor

@ryancolley Unfortunately, we've had a longer delay than anticipated due to design decisions and changes. However, we're hoping to have it out as an experimental feature by the end of this month (hopefully)!

@polatengin polatengin changed the title [Design Proposal] Modular Parameters [Design Proposal] Extendable Param Files feature Jun 27, 2024
@WithHolm
Copy link

WithHolm commented Jun 28, 2024

I think this new design change i a breakaway from the traditional way of thinking about defining parameters, and i think it will be worse because of it.

I am a huge fan of any input being explicit for any deployment, so when you look at a bicep + pram file you can se what actual input parameter is being used and what is still missing.. or optionals being set/not set.
with this you now have:
a: several param files

b: value set in a 'shared' param file will be set at deploytime, even if you didn't want to set it.

from my experience you should be able to extend the definition of a thing, but not the invocation/implementation (class can be expanded using inheritance or extended in js/typescript, but once you call that class you have to provide all required arguments)

now i'm not against having a shared param structure that can set commonly defined values, however this should be a "opt in", like you already do in "import":

// base.bicepparam
using none

param foo = 'foo'
param bar = 'bar'

// main.bicepparam
using 'main.bicep'

import * as shared from 'base.bicepparam'

param foo = 'bar'
param baz = foo
param bar = shared.bar
param other = shared.foo

// compiled.json
{
  foo: 'bar'
  bar: 'bar'
  baz: 'bar'
  other:'foo'
}

this way it is no question as to what param is being used where.

now if the reason is to provide different arguments depending on the environment, why not mplement env var checks? setting a env:bicep_param_{key}=somevalue combined with a param foo = environmentVariable({key}) would be really nice, but thats not part of this discussion

@anthony-c-martin
Copy link
Member

@WithHolm isn't the capability you're asking for already supported today?

  • main.bicepparam
    using 'main.bicep'
    
    import * as shared from './shared.bicep'
    
    param foo = 'bar'
    param baz = foo
    param bar = shared.bar
    param other = shared.foo
  • main.bicep
    param foo string
    param baz string
    param bar string
    param other string
  • shared.bicep
    @export()
    var foo = 'foo'
    
    @export()
    var bar = 'bar'

To answer some of the concerns you raised:

a: several param files

True, but it's a choice - only if the user wants to structure their code this way. IMO the following semantics are important to avoid surprise:

  • Overriding should require an explicit opt-in; accidental overriding would result in an error.
  • Extraneous parameters (supplied, but not used in .bicep) should result in an error.
  • Missing parameters should result in an error.

b: value set in a 'shared' param file will be set at deploytime, even if you didn't want to set it.

Given the above semantics, I can only see this causing a problem in the case where the user wants to rely on the parameter default value in the .bicep file, but has accidentally overridden it in the shared.bicepparam file - all other cases would result in an error or an explicit opt-in.

In this example, the behavior doesn't feel particularly surprising to me. If the user has opted in to using default values, they have already opted to move away from explicitly setting parameter values in their .bicepparam file, and have moved values into a different file (the .bicep file).

@slavizh
Copy link
Contributor

slavizh commented Jul 9, 2024

My feedback from the pull request:

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.

@slavizh
Copy link
Contributor

slavizh commented Jul 9, 2024

My feedback after watching the latest demo:

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.

@anthony-c-martin
Copy link
Member

@slavizh - thank you for sharing your thoughts! @stephaniezyen is going to work on organizing all feedback we're receiving on this new capability, and use this to prioritize further changes.

@stephaniezyen
Copy link
Contributor

@slavizh - Hi Stan, I've opened issues for each of the features you listed - like Anthony said, we're planning on putting together feedback on improvements we want to make for Extendable Param Files as well as .bicepparam files as a whole.

I failed to mention in the community call, so that's my mistake, but this is an experimental feature for now! We want to receive all feedback on usage and any improvements we may want to make before making it a public feature like you mentioned.

@slavizh
Copy link
Contributor

slavizh commented Jul 10, 2024

@stephaniezyen Thanks Stephanie!

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

Successfully merging a pull request may close this issue.