bicep icon indicating copy to clipboard operation
bicep copied to clipboard

Bad naming when decompiling

Open sebader opened this issue 3 years ago • 7 comments

Is your feature request related to a problem? Please describe. Decompilation creates some weird names in cases like this:

"variables": {
   "vnetName": "[concat(parameters('nameprefix'), 'vnet')]"
}
...
{
     "type": "Microsoft.Network/virtualNetworks",
     "name": "[variables('vnetName')]",
      "location": "[parameters('location')]",
      "apiVersion": "2020-05-01",

Produces:

var vnetName_var = '${nameprefix}vnet'

resource vnetName 'Microsoft.Network/virtualNetworks@2020-05-01' = {
  name: vnetName_var

Describe the solution you'd like Not sure if special handling of ARM variables/parameters which contain the word "name" is useful, but the current way doesn't look very nice.

sebader avatar Feb 22 '21 11:02 sebader

Thanks for reporting! I've also noticed that this is a pretty common pattern in the quickstarts repo, so would be good to try and do something about it.

It sounds like (as you're suggesting) to fix this we'd need some heuristics to handle a naming clash between a var and a resource, where the symbol ends in 'name' - for the resource we can strip the 'name' suffix, and for the var use the symbol as-is to resolve the conflict.

anthony-c-martin avatar Feb 22 '21 14:02 anthony-c-martin

After actually spending some time with a decompiled template, I should add that this is actually not just "not looking nice" but really confusing: When you then want to refer to something in your template, you really start to mix things up between using vnetName.Name, vnetName_var and vnetName.whatever.

So I ended up manually renaming all of them.

sebader avatar Feb 22 '21 18:02 sebader

One thing to add is the Bicep VS Code extension supports "rename symbol" so you should only have to rename each identifier once and bicep will update all the uses of it, but that is separate from the core issue.

alex-frankel avatar Feb 22 '21 19:02 alex-frankel

doesn't it already?! I used it and that's the thing it made it usable after all ;)

sebader avatar Feb 22 '21 20:02 sebader

To whomever will be implementing fix for this: have in mind that in ARM parameter and variable with same name could co-exist.

In addition, if the name of the resource was only a reference to variable, decompiler could skip creating variable and use variable name as the resource object (with stripped name suffix) and in other places - use i.e. vnet.name. If parameter is used, we'd need to create parameter as well, but vnet.name could also be used instead parameter.

miqm avatar Feb 22 '21 20:02 miqm

@anthony-c-martin happy to pick this one up as well.

Love the idea of stripping out variables only used for naming resources as looking through some examples in the quickstarts it is done so they can easily reference the resource again through referenceId and dependsOn which we don't need in bicep.

That being said do we think this is enough to remove the bulk of naming clashes across variables and resources and leave the existing _var convention in for scenarios not caught by the removing of the variable?

JimPaine avatar Sep 20 '21 20:09 JimPaine

Okay, so we're now removing trailing "Name" from resource symbolic names. In the https://github.com/Azure/azure-quickstart-templates repo, this removed 80% of "_var" variables and 90% of "_resource" resources.

That might be enough, please let me know after this is released if there are other common scenarios we should improve. Thanks! (Same goes for decompilation, period, please let us know what common issues you are seeing.)

StephenWeatherford avatar Sep 29 '22 17:09 StephenWeatherford

Closing for now. Please respond or open a new issue if you feel there are more improvements needed, thanks!

StephenWeatherford avatar Nov 01 '22 19:11 StephenWeatherford