Wait and Retry decorators with Experimental Feature Flag Enabled #16167
Conversation
|
Test this change out locally with the following install scripts (Action run 12897586152) VSCode
Azure CLI
|
| { | ||
| if (retryCountSyntax.Expression is not IntegerLiteralSyntax) | ||
| { | ||
| diagnosticWriter.Write(DiagnosticBuilder.ForPosition(retryCountSyntax).NegativeRetryCount()); |
There was a problem hiding this comment.
[nit]: The method name and error scenario don't quite match. This diagnostic will be written for negative integers (which will be instances of UnaryOperationSyntax), but it will also be written for expressions like 'foo' (StringSyntax), true (BooleanLiteralSyntax), 1 + 1 (BinaryOperationSyntax), foo (VariableAccessSyntax), etc.
There was a problem hiding this comment.
Take a look how batchSize is handled at L1796 above. That code is assuming that the assignability check is done elsewhere and only checks range if it's an integer value. This will also eliminate one of the new diagnostics.
|
|
||
| public Diagnostic NegativeRetryCount() => CoreError( | ||
| "BCP410", | ||
| $"Invalid retry count. It must be a non-negative integer."); |
There was a problem hiding this comment.
| $"Invalid retry count. It must be a non-negative integer."); | |
| $"Retry count must be a non-negative integer."); |
| new ObjectExpression(null, [.. optionProperties]) | ||
| ); | ||
|
|
||
| return declaredResourceExpression with { Options = options }; |
There was a problem hiding this comment.
I would keep retry and wait as separate properties on DeclaredResourceExpression because the evaluators will be called based on the order they appear in the template, and you don't want them to overwrite each other's values. E.g., for resource a in the following, the evaluator for waitUntil will be called first, then the evaluator for retryOn, and the order will be reversed for resource b.
@waitUntil(...)
@retryOn(...)
resource a ...
@retryOn(...)
@waitUntil(...)
resource b ...I would also say that grouping both into an "options" object feels tightly coupled to how this information is represented in ARM JSON rather than to how this information is expressed in Bicep.
There was a problem hiding this comment.
+1 I think modeling it as separate properties instead of one Options object in the IR makes the code more readable as well. On top of that it moves the responsibility for writing template JSON (as a single options property) back to the TemplateWriter.
| @@ -1871,6 +1871,15 @@ public Diagnostic ResourceParameterizedTypeIsDeprecated(ParameterizedTypeInstant | |||
| public Diagnostic TypeExpressionResolvesToUnassignableType(TypeSymbol type) => CoreError( | |||
| "BCP411", | |||
| $"The type \"{type}\" cannot be used in a type assignment because it does not fit within one of ARM's primitive type categories (string, int, bool, array, object).{TypeInaccuracyClause}"); | |||
|
|
|||
| public Diagnostic NegativeRetryCount() => CoreError( | |||
| "BCP410", | |||
| @@ -1524,6 +1527,11 @@ private static IEnumerable<NamespaceValue<Decorator>> GetSystemDecorators(IFeatu | |||
| { | |||
| static SyntaxBase SingleArgumentSelector(DecoratorSyntax decoratorSyntax) => decoratorSyntax.Arguments.Single().Expression; | |||
|
|
|||
| static FunctionArgumentSyntax? GetArgumentByPosition(DecoratorSyntax decoratorSyntax, int position) | |||
|
|
||
| yield return new DecoratorBuilder(LanguageConstants.RetryOnPropertyName) | ||
| .WithDescription("Causes the resource deployment to retry when deployment failed with one of the exceptions listed") | ||
| .WithRequiredParameter("list of exceptions", LanguageConstants.Array, "List of exceptions.") |
|
|
||
| yield return new DecoratorBuilder(LanguageConstants.RetryOnPropertyName) | ||
| .WithDescription("Causes the resource deployment to retry when deployment failed with one of the exceptions listed") | ||
| .WithRequiredParameter("list of exceptions", LanguageConstants.Array, "List of exceptions.") |
Users are facing issues with certain RPs (example SRP), where due to latency in replication or RP itself responded with success even before the resource is ready for use.
In such a case, dependent resource deployments fail, because the resource isn't available according to ARM or some properties of the resource isn't available like list keys for example. Issue thread for reference.
For overcoming this issue WaitUntil and RetryOn decorators are being introduced, that can apply to resource types. WaitUntil, will wait for the resource to be ready until certain properties are set to desired value, while RetryOn will retry the resource deployment when listed error codes are encountered
Microsoft Reviewers: Open in CodeFlow