Updated templates to use symbolicName.id instead of resourceId - Part 2 #3780
Conversation
bhsubra
requested review from
anthony-c-martin,
AamirAllaq,
majastrz and
MarcusFelling
and removed request for
AamirAllaq
| @@ -53,18 +67,18 @@ resource loadBalancerExternal 'Microsoft.Network/loadBalancers@2020-11-01' = { | |||
| name: 'name' | |||
| properties: { | |||
| frontendIPConfiguration: { | |||
| id: resourceId('Microsoft.Network/loadBalancers/frontendIPConfigurations', 'name', 'name') | |||
| id: 'id' | |||
| } | |||
There was a problem hiding this comment.
Why were a bunch of these broken down to just "id"? Shouldn't all of these reference symbolic name of existing resource?
There was a problem hiding this comment.
Sorry should have left a comment earlier.
I thought this wasn't a valid one-Microsoft.Network/loadBalancers/frontendIPConfigurations
- I don't see any information about this here- https://github.com/Azure/bicep-types-az/blob/main/generated/index.md
- No completion entry available
There was a problem hiding this comment.
Same about the other ones I updated in this PR. Let me know if I am missing something.
| resource vnet 'Microsoft.Network/virtualNetworks@2021-02-01' existing = { | ||
| name: /*${1:'name'}*/'name' | ||
| } | ||
|
|
||
| resource /*${2:'subnet'}*/subnet 'Microsoft.Network/virtualNetworks/subnets@2021-02-01' existing = { | ||
| parent: vnet | ||
| name: /*${3:'name'}*/'name' | ||
| } | ||
|
|
||
| resource /*${4:backendAddressPool}*/backendAddressPool 'Microsoft.Network/loadBalancers/backendAddressPools@2021-02-01' existing = { | ||
| name: /*${5:'loadBalancerExternal/loadBalancerBackEndPool'}*/'loadBalancerExternal/loadBalancerBackEndPool' | ||
| } |
There was a problem hiding this comment.
I don't think it makes sense to include this in the snippet itself - most of the time I'd expect the user to already have references to these resources.
I'd expect the snippet to be either:
subnet: {
id: /*${5:subnetRef}*/subnetRef.id
}Or:
subnet: {
id: /*${5:subnetId}*/subnetId
}There was a problem hiding this comment.
If we don't include the snippet, then we'll have to do below(include single quotes) to avoid errors:
subnet: {
id: /*${5:'subnetId'}*/'subnetId'
}
We expect user to remove quotes and update the reference. Is that correct?
There was a problem hiding this comment.
I don't understand why we'd want to keep the quotes in the snippet - does it matter whether or not the snippet itself is error-free? I feel like it would be better to cater to the snippet user rather than the snippet author, and I can't think of any scenarios where quotes would be desirable for an id field.
There was a problem hiding this comment.
Hmm my understanding was we wanted to keep the snippet error-free.
As part of #2944, I even added test to enforce this
Correct me if I am wrong.
Adding @MarcusFelling and @majastrz as well.
There was a problem hiding this comment.
As discussed offline, removed referenced snippets:
c4c04eb
Also, included quotes as part of snippet placeholder:
| properties: { | ||
| frontendIPConfiguration: { | ||
| id: resourceId('Microsoft.Network/loadBalancers/frontendIPConfigurations', /*${2:'name'}*/'name', /*${3:'name'}*/'name') | ||
| id: 'id' |
There was a problem hiding this comment.
Here I think the original approach was optimal - resourceId('Microsoft.Network/loadBalancers/frontendIPConfigurations', /*${2:'name'}*/'name', /*${3:'name'}*/'name')
Unfortunately I don't think there's a nice way to simplify this - as I think you mentioned on another comment, this isn't a real resource; it's a means of self-referencing properties within a single resource body that Microsoft.Network created to fit their scenario.
There was a problem hiding this comment.
Made the change you suggested above here as well:
c4c04eb
| enableFloatingIP: false | ||
| idleTimeoutInMinutes: 5 | ||
| probe: { | ||
| id: resourceId('Microsoft.Network/loadBalancers/probes', /*${2:'name'}*/'name', /*${12:'name'}*/'name') | ||
| id: 'id' |
There was a problem hiding this comment.
Same here - unfortunately I don't think we have a way to avoid the original syntx.
There was a problem hiding this comment.
Oops I forgot to add placeholder here. So I am guessing we want here(per your below comment):
/*${12:'probesId'}*/'probesId'
There was a problem hiding this comment.
Made the change you suggested in above comment:
c4c04eb
| containedResources: [ | ||
| resourceId('Microsoft.OperationalInsights/workspaces/views', /*${3:'logAnalyticsWorkspace'}*/'logAnalyticsWorkspace', /*${4:'logAnalyticsSolution'}*/'logAnalyticsSolution') | ||
| /*${5:'id'}*/'id' |
There was a problem hiding this comment.
I think you can go with one of either:
/*${5:viewRef}*/viewRef.idOr:
/*${5:viewId}*/viewIdThere was a problem hiding this comment.
Updated here:
c4c04eb
Fixes #3501
Updated templates to use symbolicName.id instead of resourceId