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

Bicep deploy - additional params support #6159

Closed
bhsubra opened this issue Mar 8, 2022 · 11 comments
Closed

Bicep deploy - additional params support #6159

bhsubra opened this issue Mar 8, 2022 · 11 comments
Assignees
Labels
devdiv Related to Bicep tooling efforts in DevDiv enhancement New feature or request P1 This is planned to be completed before the end of a release V2 This is seen as necessary, but only covering all V1s of the initial feature
Milestone

Comments

@bhsubra
Copy link
Contributor

bhsubra commented Mar 8, 2022

Add support to ask for any additional params if they're not specified in the params file

Related to #1862

@bhsubra bhsubra added enhancement New feature or request devdiv Related to Bicep tooling efforts in DevDiv V2 This is seen as necessary, but only covering all V1s of the initial feature labels Mar 8, 2022
@bhsubra bhsubra self-assigned this Mar 8, 2022
@ucheNkadiCode ucheNkadiCode added this to the v0.6 milestone Mar 30, 2022
@ucheNkadiCode ucheNkadiCode moved this to Todo in Bicep Mar 30, 2022
@ucheNkadiCode
Copy link
Contributor

ucheNkadiCode commented Apr 1, 2022

This would be the case where, if the user has any param values in their main Bicep file, we add this dynamically to the flow of the deploy command.

  1. First we ask for a parmaters.json file
  2. Ask for any of the params that are not within this file - we would prepopulate the values if they added a default (e.g location string = resourceGroup().location)
  3. Run the deployment

@bhsubra
A few things to figure out:

  • Is there anything special we need to do for modules? If you are deploying a main.bicep file and it has various modules, it is my understanding we won't have to parse the module files to get any param values
  • Should we still ask the user for the param values found in the parameters.json file? My choice would be... no, because they went through the effort of making such file.

@ucheNkadiCode ucheNkadiCode added the P1 This is planned to be completed before the end of a release label Apr 1, 2022
@stephaniezyen stephaniezyen modified the milestones: v0.6, v0.7 Apr 22, 2022
@bhsubra bhsubra moved this from Todo to In Progress in Bicep Apr 28, 2022
@ucheNkadiCode
Copy link
Contributor

@bhsubra this will be our P1 first to-do for the 0.7 timeline

@anthony-c-martin
Copy link
Member

Wouldn't it be better to focus efforts on #512 / #3381, with additional support for "add missing parameters to params file"

I'd have thought users would either have a params file, or not - if they have a 'partial' params file, wouldn't they want to update it rather than leaving it partially complete?

@ucheNkadiCode
Copy link
Contributor

ucheNkadiCode commented May 17, 2022

@anthony-c-martin Here's what we plan to do. We've chatted with Engin and he mentioned that his PR #6601 for generating a param file from a bicep file simply creates a new parameters file or overwrites an existing file of that name. @polatengin mentioned that if it were suggested, that PR could be modified to also gracefully update a file's generated parameter file if it already existed, especially if more params got added to the Bicep file, but I think that is additional behavior for that use case. I believe that his PR is sufficient to help with a user's scenario of wanting a parameters file. I think we should leave it up to a user if they for some reason have a partial params file, if they want to update it to include all of the parameters in the Bicep file or better yet, use @polatengin's new PR feature, that's their choice.

  1. First we ask for a parameters.json file
  2. We then ask for any params in the Bicep file that were not included in the parameters.json selected
  3. Parameters that have a default expression (resourceGroup.location), will have a placeholder text in the dialog then helpful text underneath will say "Press enter to use default"
  4. From Bhavya's existing demo: We will get rid of that row in the parameters section that says "Enter value for parameter"
  5. From Bhavya's existing demo: We will not ask for any parameters that are already in the parameters.json file
  6. We will use Quick pick instead of input box because it has visual steps 2/9 and a back button
  7. If a parameter has a default value in the Bicep file AND it is not included in the parameters.json file, we will prefill the text box with it then add a message underneath that says "Press enter to use default"
  8. If it does not have a default value, it will be empty and we will have a placeholder with a message underneath that says "Enter value for {parameter name}

@anthony-c-martin
Copy link
Member

Oh, nice - I totally missed that we had a PR out to contribute #512!

Thanks for the detailed explanation; if we have a demo of the above, I'd love to see it! Some thoughts/questions:

  1. Bicep files with a lot of defaults are quite common - picking something at random, see here. It would be great to ensure the UI doesn't require too many clicks to deploy a file with many defaults.
  2. It would be a shame to take all this user input and not persist it somewhere - feels like it would be worth offering to save the overridden parameters back to the parameters.json file after deploying.
  3. I'd expect any parameters file checked into source control to be 'complete' - is this feature about simplifying dev-time experimentation, or are there scenarios that the user would want to check in a partial params file?

@ucheNkadiCode
Copy link
Contributor

ucheNkadiCode commented May 18, 2022

If we go with anthony's suggestion of caching, this may be similar to -> #6222
Basically as we make the addition of the issue above, for any of the values that a user inputs (meaning it wasn't included in the parameters.json) that doesn't have a Bicep default explicitly laid out in the file, we are going to pre-fill that parameter

Thanks for your suggestion Anthony, we'll be asking the user if they want to update the parameters file. This would only happen IF a parameters file was involved in their deployment.

@bhsubra and @anthony-c-martin, @stephaniezyen or @alex-frankel , Do we want this to be a VS Code pop up in the bottom right corner with the option of don't ask me again? "Would you like to update your [parameters.json] file to include the values declared for these missing parameters?" [Yes] [No] [Don't Ask me again]

Or should it be included in the dialog flow?

It feels like since it's a "smart option" it belongs in a pop-up seperate from the actual dialog each time

@bhsubra
Copy link
Contributor Author

bhsubra commented May 23, 2022

Some questions regarding parameters with default values:

  1. For param of type array, e.g. https://github.com/Azure/azure-quickstart-templates/blob/d8686ed40d9476213635d602798e9a1045f63579/quickstarts/microsoft.signalrservice/signalr/main.bicep#L42, do we want to display the default value to the user? There might be readability issues if there are too many values in the array. I don't see an option here: https://code.visualstudio.com/api/references/vscode-api#InputBoxOptions that supports scrolling

  2. Same issue as above for param of type object, e.g.

    param resourceTags object = {

  3. What should be the format of the input in above cases? Should it be in the format that is supported by bicep file or generated arm template? I prefer generated arm template format as we generate it before we start the deployment. Bicep file format would require us to do another round of build, validation (Note: azure-sdk-for-net requires an arm template for deployment)

  4. Should we ease the format of input? E.g. for array type, should the input be of the format

[
  "https://foo.com",
  "https://bar.com"
] 

or should we allow below format?

"https://foo.com", "https://bar.com"

I am a bit hesitant to ease the format for any of the types as it might end up complicating things. E.g. when param is of type object

resource blueprintName_policyArtifact 'Microsoft.Blueprint/blueprints/artifacts@2018-11-01-preview' = {
  parent: blueprintName_resource
  name: 'policyArtifact'
  kind: 'policyAssignment'
  properties: properties
}

param properties object = {
  displayName: 'Blocked Resource Types policy definition'
  description: 'Block certain resource types'
  policyDefinitionId: tenantResourceId('Microsoft.Authorization/policyDefinitions', '6c112d4e-5bc7-47ae-a041-ea2d9dccd749')
  resourceGroup: 'sampleRg'
  parameters: {
    listOfResourceTypesNotAllowed: {
      value: '[parameters(\'listOfResourceTypesNotAllowed\')]'
    }
  }
}

@anthony-c-martin
Copy link
Member

4. Should we ease the format of input? E.g. for array type, should the input be of the format

I'm not suggesting this is the best option, but here's what the Azure Portal does for array/object params:
image

@bhsubra
Copy link
Contributor Author

bhsubra commented May 27, 2022

Discussed this in team sync up last week. For v1, this is what we plan to do:

  • We will ignore param of types – array and object
  • For expression, we’ll provide an option to either use existing expression, leave blank or option to provide a new value
  • After the users have provided values for missing params and ones that have defaults, we’ll show an option to either update parameters.json file or create one if it doesn’t exist

@bhsubra
Copy link
Contributor Author

bhsubra commented May 27, 2022

  1. Should we ease the format of input? E.g. for array type, should the input be of the format

I'm not suggesting this is the best option, but here's what the Azure Portal does for array/object params: image

@anthony-c-martin , thanks for the input. We plan to not support array and objects in first iteration because of the complexities involved. Let me know if you have any concerns. Thanks!

cc: @ucheNkadiCode

@ucheNkadiCode
Copy link
Contributor

Related to #6158 since we currently force the user to browse each time they deploy a bicep file

@stephaniezyen stephaniezyen moved this from In Progress to In Review in Bicep Jun 2, 2022
@bhsubra bhsubra closed this as completed Jun 7, 2022
Repository owner moved this from In Review to Done in Bicep Jun 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
devdiv Related to Bicep tooling efforts in DevDiv enhancement New feature or request P1 This is planned to be completed before the end of a release V2 This is seen as necessary, but only covering all V1s of the initial feature
Projects
Archived in project
Development

No branches or pull requests

4 participants