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 VS Code extension should show/remove properties with default values #2479

Open
NeilMacMullen opened this issue Apr 30, 2021 · 9 comments
Labels
bpa-parity devdiv Related to Bicep tooling efforts in DevDiv enhancement New feature or request story: linter rules

Comments

@NeilMacMullen
Copy link

NeilMacMullen commented Apr 30, 2021

Is your feature request related to a problem? Please describe.
I'm desperately trying to migrate away from a set of poorly-understood ARM templates. It's great that there are a bunch of example BICEP projects in the repo but the vast majority appear to be uncommented and suffer from the same issue as ARM templates - i.e every possible parameter has been filled out with no indication of which are defaults and which are "important".

In contrast, when I create a resource in the Azure Portal, it's (generally) quite simple to leave everything at default settings and have the UI guide you through the few mandatory changes.

Describe the solution you'd like

This is just a suggestion and perhaps there are better ways to do this but:

  • The VS Code extension could provide different styling for parameters which have a required non-default value (e.g. names) or a strongly encouraged non-default value (e.g. locations).
  • The VS Code extension could provide different styling for parameter values which are at their default (factory) setting
  • The VS Code extension could provide tooltip descriptions for parameter names and values.

For example when I look at https://github.com/Azure/bicep/blob/main/docs/examples/101/vm-simple-windows/main.bicep (which seems fairly representative) it's entirely unclear whether

var addressPrefix = '10.0.0.0/16'

is being set for some specific reason or if it's just the standard default. And if it's the default, why can't I just write

resource nInter 'Microsoft.Network/networkInterfaces@2020-06-01' = {
  name: nicName
  location: location
}

and have the properties block defaulted?

@NeilMacMullen NeilMacMullen added the enhancement New feature or request label Apr 30, 2021
@ghost ghost added the Needs: Triage 🔍 label Apr 30, 2021
@alex-frankel
Copy link
Collaborator

In general, we encourage examples that we review (in this repo and in ARM Template Quick Starts) to only include non-defaulted properties, so you should assume it is there because it needs to be. If you export a template from the portal you will get a lot of superfluous default properties, so take those templates for what they are worth. Personally, I'd only recommend using them to discover the shape of a particular resource, but not use it directly in production.

In that particular example, the addressPrefix is required. There is no way to have it set for you automatically. The confusing part here is that this particular resource type does not do a great job defining required properties in their REST API spec (which is what powers the bicep types), so bicep will tell you it's not required even though it is.

Where would you like the information you are asking for to be surfaced? When you bring up completions while authoring a resource, we will tell you which properties are required:
image

@NeilMacMullen
Copy link
Author

NeilMacMullen commented Apr 30, 2021

In that particular example, the addressPrefix is required. There is no way to have it set for you automatically.

i don't doubt you but I'm pretty certain I have never had to explicitly set this value when creating a VM in the portal. "As an Azure user" why would I suddenly have to start worrying about it when using BICEP? (As a developer I'm sympathetic to the technical difficulties of getting the information via the REST spec but that seems like a problem the Azure team could reasonably resolve?)

Where would you like the information you are asking for to be surfaced?

My suggestion was slightly different - use the knowledge you have to influence the styling the VS code extension applies. At the moment I believe the styling is based on the type of value whereas it could be based on the kinds of parameters I described. Here's a clumsy powerpoint mockup where I've bolded and underlined required fields and colored "default" values gray and modified values red.

image

To me, this makes it far easier to ignore the boilerplate defaults and scan a template to see what has been changed.

@alex-frankel
Copy link
Collaborator

I'm pretty certain I have never had to explicitly set this value when creating a VM in the portal.

Nailed it. The portal client is abstracting these details away from you. I suspect this will be solved with the #2128 in which Microsoft (and others) can publish a canonical interface (via a module) for a particular resource or set of resources to abstract this away. In a sense, the portal UX (and a PS or CLI client) is analogous to a built-in module in the registry. They are both hand-authored and designed to make consumption of that resource(s) as easy as possible.

WRT the VS code ux for properties, that mockup helped clarify. I guess what I'm stuck on is that all the properties with default values shouldn't even be there - they are just noise. IOW, why not remove all the properties with gray values? There's probably something that could be added to the linter (#2341) to catch "properties with values set to the default value" and provide a quick fix to remove it. (FYI @MarcusFelling @jfleisher).

@NeilMacMullen
Copy link
Author

NeilMacMullen commented May 3, 2021

guess what I'm stuck on is that all the properties with default values shouldn't even be there - they are just noise. IOW, why not remove all the properties with gray values?

No argument there, you're actually going further than I hoped for :-) Tbf, it is sometimes useful to see what could be changed and you still need a styling policy for values which are present but changed back to their default value. Another possibility would be to use folding to hide blocks of default values.

@alex-frankel alex-frankel added story: linter enhancement New feature or request and removed awaiting response labels May 3, 2021
@MarcusFelling MarcusFelling added this to the v0.4 milestone May 3, 2021
@MarcusFelling MarcusFelling added bpa-parity and removed enhancement New feature or request labels Aug 23, 2021
@StephenWeatherford StephenWeatherford added the devdiv Related to Bicep tooling efforts in DevDiv label Jan 21, 2022
@ucheNkadiCode
Copy link
Contributor

Related to #5316

@StephenWeatherford
Copy link
Contributor

@NeilMacMullen I'm changing the title of this bug to reference just the default properties issue. Above, though, you also say:

The VS Code extension could provide tooltip descriptions for parameter names and values.
I think this functionality already exists, e.g.:
image
Or is it something else you meant? (If so, I'd prefer a separate issue to be entered and referenced here). Thanks!

@StephenWeatherford StephenWeatherford changed the title Suggestions for making Bicep VS Code extension more beginner-friendly Bicep VS Code extension would be more beginner-friendly if it showed/allowed removing properties with default values Dec 7, 2022
@StephenWeatherford StephenWeatherford added the enhancement New feature or request label Dec 7, 2022
@StephenWeatherford StephenWeatherford changed the title Bicep VS Code extension would be more beginner-friendly if it showed/allowed removing properties with default values Bicep VS Code extension should show/remove properties with default values Dec 7, 2022
@stijnherreman
Copy link

The confusing part here is that this particular resource type does not do a great job defining required properties in their REST API spec (which is what powers the bicep types), so bicep will tell you it's not required even though it is.

Are resource types responsible for defining the properties property as required? Or is this something that bicep should assume?

For example, https://learn.microsoft.com/en-us/azure/templates/microsoft.insights/actiongroups?pivots=deployment-language-bicep mentiones that some ActionGroup properties are required (enabled and groupShortName) but it does not mention that the root properties itself is required. So you could assume that some default values will be applied, but trying to deploy will return an error "The ActionGroup request is null or empty".

@alex-frankel
Copy link
Collaborator

I don't believe Bicep makes any assumptions about this and we rely on the swagger to capture this, but @anthony-c-martin / @jeskew would know for sure.

@jeskew
Copy link
Member

jeskew commented Mar 28, 2024

Bicep does rely on resource provider service models to determine what properties are required. properties is not marked as optional in the ActionGroups body definition, and Bicep is just reflecting what's in the model.

There are resource types that don't require the properties property (Micrsoft.Storage/storageAccounts and Microsoft.ManagedIdentity/userAssignedIdentities come to mind), so I wouldn't want Bicep to just assume that not marking properties as required is a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bpa-parity devdiv Related to Bicep tooling efforts in DevDiv enhancement New feature or request story: linter rules
Projects
None yet
Development

No branches or pull requests

7 participants