-
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 snippets to bicep - part 2 #2207
Conversation
@anthony-c-martin @majastrz mind reviewing? Hoping to get this merged before publishing a contribution guide. |
I noticed a bug in requesting completions for the snippets - following is the repro. I was testing this with the
EDIT: I'll raise a bug for this, as I think it's unrelated to this set of changes. |
@@ -0,0 +1,10 @@ | |||
// Log Analytics Workspace | |||
resource logAnalyticsWorkspace 'Microsoft.OperationalInsights/workspaces@2015-11-01-preview' = { |
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.
The symbolic name here is fixed, but it feels like customers will most likely want to change this. Should we offer a placeholder on the symbolic name?
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.
One challenge here is that it will make the authoring of the snippet bicep file harder because we'll get a parse error. However, my understanding was that we were going to have some sort of convention-based replacement. I see that the names of name
placeholders do match the symbolic names, but I guess the logic to turn symbolic names into placeholders isn't there yet? @bhsubra?
In reply to: 611932375 [](ancestors = 611932375)
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.
@majastrz , do you mean named placeholder for name E.g: ${name: logAnalyticsWorkspace}? We currently don't have that support, but I can try to add some logic to support the same.
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.
I'll try to make this change in a follow up PR. Let me know if you have any concerns.
src/Bicep.LangServer/Snippets/Templates/res-logic-app-connector.bicep
Outdated
Show resolved
Hide resolved
src/Bicep.LangServer/Snippets/Templates/res-logic-app-connector.bicep
Outdated
Show resolved
Hide resolved
...angServer.IntegrationTests/Completions/SnippetTemplates/res-keyvault-secret/diagnostics.json
Outdated
Show resolved
Hide resolved
7372ba7
to
7a6d8ca
Compare
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.
Looks awesome! Thanks for doing this!
Port arm snippets - part 2