-
Notifications
You must be signed in to change notification settings - Fork 764
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
Port arm tools snippets - part 6 #2782
Conversation
name: 'name' | ||
properties: { | ||
subnet: { | ||
id: resourceId('Microsoft.Network/virtualNetworks/subnets', 'virtualNetwork', 'Subnet-1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we encourage symbolic references to other resources rather than using the resourceId()
function in snippets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anthony-c-martin, I don't think we have templates checked in for most of the 'resourceProvider/type' listed in resourceId(..).
I had an offline discussion with @MarcusFelling regarding this. I am going to update it to be a tab stop instead. Do you have any concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bhsubra - maybe this could be a topic in the next DSL discussion? I'd definitely like to make sure we're encouraging best practices with the snippets, as they're going to be an entry-point to the language and serve as a learning opportunity for many new users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not disagreeing with that. I suggested we remove the use of resourceId function in all snippets and instead have a tab stop/placeholder where symbolic name can be used. We can't reference another resource's symbolic link in a single resource snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use placholders here:
ea170a6
location: resourceGroup().location | ||
tags: { | ||
Application_Type: '${3|web,other|}' | ||
'hidden-link:${resourceGroup().id}/providers/Microsoft.Web/serverfarms/${4:appServicePlan}': 'Resource' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be something like:
'hidden-link:${appService.id}': 'Resource'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here:
ea170a6
{ | ||
metricTrigger: { | ||
metricName: ${10:'name'} | ||
metricResourceUri: '${resourceGroup().id}/providers/Microsoft.Web/serverfarms/${4:appServicePlan}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, use a symbolic ref to the app service:
metricResourceUri: appService.id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here:
ea170a6
name: ${32:'name'} | ||
properties: { | ||
subnet: { | ||
id: resourceId('${33:virtualNetworkResourceGroup}', 'Microsoft.Network/virtualNetworks/subnets', '${34:virtualNetworkName}', 'AzureFirewallSubnet') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id: subnet.id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subnet symbolic name does not exist in the context of this snippet. I suggested we have tabstops instead and remove the use of resourceId function in all snippets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here:
ea170a6
5c04c01
to
61f033f
Compare
Port remaining arm tools snippets