fix for bicep issue #15458 #16237
fix for bicep issue #15458 #16237
Conversation
|
Test this change out locally with the following install scripts (Action run 13190289530) VSCode
Azure CLI
|
Dotnet Test Results 78 files - 39 78 suites - 39 31m 51s ⏱️ - 17m 29s Results for commit dea5122. ± Comparison against base commit 93c5975. This pull request removes 1795 and adds 615 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
@ouldsid please could you add a test to cover the scenario reported in the original issue. This is just to ensure we don't regress the behavior again in the future. |
There are two tests covering this scenario they did not fail because an unrelated issue was causing the test not to run (I ran into the same issue locally -> Asked Shenglong and he pointed out that he fixed it with a change that was merged yesterday I then merged with main and push a new commit (number 2 in this PR) to make sure that these test fail once this was verified I changed the test to behave as expected |
| var value = PlaceholderParametersBicepParamWriter.GetValueForParameter(parameterSymbol.DeclaringParameter); | ||
|
|
||
| jsonWriter.WriteStartObject(); | ||
| switch (parameterSymbol.Type.Name) | ||
| { | ||
| case "string": | ||
| emitter.EmitProperty("value", ""); | ||
| break; | ||
| case "int": | ||
| emitter.EmitProperty("value", () => jsonWriter.WriteValue(0)); | ||
| break; | ||
| case "bool": | ||
| emitter.EmitProperty("value", () => jsonWriter.WriteValue(false)); | ||
| break; | ||
| case "object": | ||
| emitter.EmitProperty("value", () => { jsonWriter.WriteStartObject(); jsonWriter.WriteEndObject(); }); | ||
| break; | ||
| case "array": | ||
| emitter.EmitProperty("value", () => { jsonWriter.WriteStartArray(); jsonWriter.WriteEndArray(); }); | ||
| break; | ||
| } | ||
|
|
||
| emitter.EmitProperty("value", value); |
There was a problem hiding this comment.
One additional behavior change this will introduce is that a default of "" will be used for any parameter using a user-defined type if I'm reading this correctly. For example, I believe the following template:
param foo {
requiredProperty: string
optionalProperty: string?
}will generate a .bicepparam file of
param foo = ''There was a problem hiding this comment.
The way to generate .bicepparam should not change. The change is to make .parameters.json consistant with it. Having said that this might be an issue but unrelated to this change
Awesome, thanks! |
| @@ -97,25 +97,16 @@ private JToken GenerateTemplate(string contentVersion) | |||
| { | |||
| jsonWriter.WritePropertyName(parameterSymbol.Name); | |||
|
|
|||
| var value = PlaceholderParametersBicepParamWriter.GetValueForParameter(parameterSymbol.DeclaringParameter); | |||
There was a problem hiding this comment.
It doesn't feel safe to me to depend on this method and then convert from Bicep -> JSON, because it can return invalid Bicep syntax - e.g. here:
bicep/src/Bicep.Core/Emit/PlaceholderParametersBicepParamWriter.cs
Lines 141 to 144 in eb0673b
My suggestion would probably be to copy the pattern used in PlaceholderParametersBicepParamWriter and introduce a dedicated method like GetValueForParameter which is able to accurately generate the right type of JSON node, including JSON comments if we're unable to generate valid syntax.
There was a problem hiding this comment.
is parameterDeclarationSyntax propre to bicep or can also be used by json ? my understanding is that : this.Context.SemanticModel.Root.ParameterDeclarations will be the same whether we are dealing with generating bicep or json file ...am I missing something? the value you pointed out will be handled by the the added if statement you have pointed out with your second comment
| switch (parameterSymbol.Type.Name) | ||
|
|
||
| // emit value property only if the text is not null | ||
| if (value.ToString() != "\r\n") |
There was a problem hiding this comment.
This check is imprecise - is it possible to add logic that more specifically detects the edge cases directly against value, rather than converting to string?
I wasn't sure exactly what patterns you were aiming to detect here, so my comment is a bit vague. I can probably give you more specific suggestions if you could explain this.
There was a problem hiding this comment.
This was the case where the value is : SyntaxFactory.NewlineToken maybe I can change the if statement to be value.Type forexample?
…x for an existing bug in ExpressionBuilder -> ConvertArray
| } | ||
| else | ||
| { | ||
| if (arrayItem.Value is not null) |
There was a problem hiding this comment.
Btw, this is used in the compilation path of .bicep files. I'm surprised we need changes here.
There was a problem hiding this comment.
The real issue was with the way the defaultValue is being returned. Fixed the real issue in the last iteration
| switch (parameterSymbol.Type.Name) | ||
|
|
||
| // emit value property only if the value is not a newline token | ||
| if (value is not Token { Type: TokenType.NewLine}) |
There was a problem hiding this comment.
Added more context to the comment
|
I've got a general suggestion for how to solve this problem without having to deal with the complexity of converting invalid Bicep syntax to JSON. The challenge we have is parameter default values can have functions anywhere, so you could have: param foo object = {
bar: 'bar'
baz: { qux: newGuid() }
}In which case, the .bicepparam conversion will give you: param foo = {
bar: 'bar'
baz: {
qux: ? // TODO...
}
}Currently you're trying to convert the value into JSON, which is extremely tricky because the compiler is intentionally not designed to compile invalid syntax. I don't see an obvious solution to this. My suggestion is instead of trying to emit the following JSON: {
"parameters": {
"foo": {
"value": {
"bar": "bar",
"baz": { "qux": // TODO }
}
}
}We just emit: {
"parameters": {
"foo": {
"value": // TODO
}
}
}I suspect this is a much easier problem to solve because you just need to check "is the Bicep syntax valid" (which can be done with a visitor), and avoiding the conversion if it is not. While this isn't true functional parity between .bicepparam and .json, I feel like it's "good enough" - ultimately, having functions in objects is quite an edge case, and IMO is not worth the added complexity to handle properly. |
…d add it to the correct location
Thank you @anthony-c-martin my attempt was to keep the same behavior for Json parameter writer (last iteration) when the syntax is not correct which was just to assign the value to empty string or empty object depending on the parameter type... but I think that your suggestion is much better I will implement it and send a new iteration |
Contributing a Pull Request
If you haven't already, read the full contribution guide. The guide may have changed since the last time you read it, so please double-check. Once you are done and ready to submit your PR, run through the relevant checklist below.
Contributing to documentation
Contributing an example
We are integrating the Bicep examples into the Azure QuickStart Templates. If you'd like to contribute new example
.bicepfiles that showcase abilities of the language, please follow these instructions to add them directly there. We can still take bug reports and fixes for the existing examples for the time being.dotnet testContributing a feature
Contributing a snippet
I have a snippet that is either a single, generic resource or multi resource that uses parent-child syntax
I have checked that there is not an equivalent snippet already submitted
I have used camelCasing unless I have a justification to use another casing style
I have placeholders values that correspond to their property names (e.g.
dnsPrefix: 'dnsPrefix'), unless it's a property that MUST be changed or parameterized in order to deploy. In that case, I use 'REQUIRED' e.g. keyDataI have my symbolic name as the first tab stop ($1) in the snippet. e.g. res-aks-cluster.bicep
I have a resource name property equal to "name"
If applicable, I have set the
locationproperty tolocation: /*${<id>:location}*/'location'(notresourceGroup().location) where<id>is a placeholder id, and addedparam location stringto the test's main.bicep file so that the resulting main.combined.bicep file used in the tests compiles without errorsI have verified that the snippet deploys correctly when used in the context of an actual bicep file
e.g.
Microsoft Reviewers: Open in CodeFlow