Move all indexing expressions when calculating dependsOn expressions
#15580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… ResourceDependencyVisitor
|
Test this change out locally with the following install scripts (Action run 12147673701) VSCode
Azure CLI
|
Dotnet Test Results 78 files - 39 78 suites - 39 30m 19s ⏱️ - 15m 14s Results for commit 851f527. ± Comparison against base commit 95ede42. This pull request removes 1840 and adds 658 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
# Conflicts: # src/Bicep.Core.IntegrationTests/ScenarioTests.cs
jeskew
force-pushed
the
jeskew/15513-bis
branch
from
74bd307 to
56d041c
Compare
|
@jeskew - are there any opportunities to incorporate the real Deployment engine's expansion/reference evaluation capabilities into our testing framework? Given there are optimizations in both Bicep and backend which need to work correctly together, it's pretty hard to look at the codegened output and say "this looks correct". It would be really useful to be able to write tests or baselines that assert the exact deployment graph which the engine would follow, and I think probably help us avoid any regressions in this logic in the longer run. |
|
@anthony-c-martin This is turning out to be much higher labor than I thought it would be. The parameters need to be written by hand, and the baselines that I have tried to convert to a deployment plan have failed validation because they're meant to exercise language features rather than be deployable. I'm going to scope this PR down to just fixing the bug in #15513 and move the fix for #13674 and #15686 to a follow-up PR. The former shouldn't affect existing baselines, and the latter is a pretty small change. |
jeskew
force-pushed
the
jeskew/15513-bis
branch
from
7aceac6 to
851f527
Compare
jeskew
changed the title
dependsOn expressions
shenglol
reviewed
Dec 3, 2024
| }; | ||
| if (expression is null) | ||
| { | ||
| #pragma warning disable IDE0301 // Using a simplified collection initialization results in an allocation, whereas using ImmutableArray<T>.Empty does not |
There was a problem hiding this comment.
Btw this will be optimized in C# 13.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #15513
Microsoft Reviewers: Open in CodeFlow
Bicep will normally generate an explicit dependency when one resource refers to another. For example, if the body of
bincludes a symbolic reference toa, then in the compiled JSON template, the declaration forbwill have adependsOnproperty that includesa.However, if
ais anexistingresource and the template is not being compiled to language version 2.0, then the compiler will "skip over"aand havebdepend on whateveradepends on. For example, for the following template:the non-symbolic name output will have
bdepend onc.#15447 added a couple of scenarios in which Bicep would skip over an existing resource even if compiling with symbolic name support. This was done because the ARM backend will perform a
GETrequest on anyexistingresource in the template unless its body properties are never read and no deployed resource explicitly depends on it. The extraGETrequests could sometimes cause template deployments to fail, for example if the deploying principal had permission to use secrets from a key vault as part of a deployment but did not have more generic /read permissions on the vault.#15447 reused some existing logic for skipping over an intermediate existing dependency that unfortunately had an underlying bug that manifested when the skipped over resource was looped and used its loop iterator to index into the dependency once removed. For example, if we modify the earlier example slightly:
Then in the compiled output,
bwill have an explicit dependency on[resourceId('type', format('c[{0}]', copyIndex()))]. Becausebis not looped, the deployment will fail. Related issues will occur ifbindexes intoawith a more complex expression or if there is an intervening variable.This PR updates explicit dependency generation to take all steps between a depending resource and its dependency into account when generating index expressions. For example, in the following template:
vaultwill depend onvnets[(range(20, 10)[k - 11] - 20) % 2]. Prior to this PR,vaultwill instead depend onvnets[k % 2], which is the wrong vnet.This PR does not reapply the change from #15447 but only addresses the issue described above. #15447 is reapplied in #15693