Implement conditional resources and modules #1014
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1014 +/- ##
==========================================
+ Coverage 94.26% 94.78% +0.52%
==========================================
Files 328 325 -3
Lines 15810 15772 -38
Branches 12 0 -12
==========================================
+ Hits 14903 14950 +47
+ Misses 907 822 -85
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -223,9 +221,14 @@ private void EmitResource(ResourceSymbol resourceSymbol) | |||
|
|
|||
| var typeReference = EmitHelpers.GetTypeReference(resourceSymbol); | |||
|
|
|||
| if (resourceSymbol.DeclaringResource.Body is IfExpressionSyntax conditionalBody) | |||
| { | |||
| this.emitter.EmitProperty("condition", conditionalBody.ConditionExpression); | |||
There was a problem hiding this comment.
Btw, do we have any limitations on what can go into resource conditions in the runtime? Are all functions including stuff like reference() or listKeys() allowed?
There was a problem hiding this comment.
Good questions. The documents didn't mention any limitations, but let me do some tests to figure out.
There was a problem hiding this comment.
My expectation was deploy-time constants only (similar to name, type, apiVersion field requirements) because the engine calculates the deployment graph upfront. We should probably make a note to clarify this in the docs once you've tested this.
There was a problem hiding this comment.
Confirmed with template deployments that runtime functions are not allowed in a resource condition (also found this ARM unit test that validates it). I'll update the code to block the use of them, and I think @anthony-c-martin is right that we should update the docs to clarify this.
There was a problem hiding this comment.
reference() and list* functions are blocked now. We still need to block resource and module symbolic name references, which I will implement in a separate PR once Tarun's PR is merged.
| public SyntaxBase ResolvedBody => | ||
| this.Body is IfExpressionSyntax conditionalBody ? conditionalBody.ConsequenceExpression : this.Body; |
There was a problem hiding this comment.
Does this require a grammar update? (grammar.md).
If I'm understanding correctly, looks like with these changes we've got something like:
resourcedecl -> "resource" identifier string "=" body
body -> conditionalBody | object
conditionalBody -> "if" "(" expression ")" object
Just wonder, rather than using recursion for the definition, would it make sense to model the grammar as:
resourcedecl -> "resource" identifier string = ifexpr? object
ifexpr -> "if" "(" expression ")"
This is similar to what we did for the param default syntax, and might make chaining (when we have loops) and recursion easier to deal with. I think it also removes the need for the ResolvedBody property.
There was a problem hiding this comment.
Yeah, I think it makes sense to not use recursion for the definition, and I like how it simplifies things a lot. The recursion version may work better if we are going to add an alternative expression to support else, but I doubt that's necessary, since if...else... can be replaced with a ternary operator. I'm going to change the grammar to the suggested version.
There was a problem hiding this comment.
Updated the grammar to not use recursion. I changed IfExpressionSyntax to IfCondtionSyntax though, because it's not an expression anymore.
src/Bicep.Core.Samples/Files/InvalidResources_CRLF/main.diagnostics.bicep
Outdated
Show resolved
Hide resolved
src/Bicep.Core/Parsing/Parser.cs
Outdated
| var conditionExpression = this.WithRecovery(() => this.ParenthesizedExpression(true), RecoveryFlags.None, TokenType.LeftBrace, TokenType.NewLine); | ||
| var @object = this.WithRecovery(this.Object, GetSuppressionFlag(conditionExpression), TokenType.NewLine); |
There was a problem hiding this comment.
Mind adding a line to the invalid resources file for something like: ... if ({'a': b}.a == true) {? It's invalid syntax, just curious to see what sort of error message results from the recovery logic here.
There was a problem hiding this comment.
Added a test case. It produces a error message like expect a ) character... at the end, which is not very helpful. I also tried a invalid parenthesized expression in a resource declartion, and got a similar error message.
Added
IfConditionSyntaxto support conditional resources and modules. The things I'm planning to do in separate PRs:IfConditionSyntaxCloses #186.