Fix Azure roles resources always redeploying - #12901
Conversation
Everytime we check if we need to redeploy an Azure "roles" resource in run mode, we are getting a different CheckSum. This is because we are overwriting the "known parameters" like PrincipalId and PrincipalType with 'null' values, because these values aren't available yet. The fix is to skip setting those known parameters, like we did in previous versions. Fix microsoft#12651
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12901Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12901" |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where Azure "roles" resources were being redeployed every time an application runs. The problem occurred because known parameters (like PrincipalId and PrincipalType) were being overwritten with null values during checksum calculation, causing the checksum to change unnecessarily.
Key Changes:
- Added a
skipKnownValuesparameter toSetParametersAsyncto prevent overwriting known parameters withnullvalues - Modified
GetCurrentChecksumAsyncto useskipKnownValues: truewhen calculating checksums - Added a test to verify known parameters are not overwritten during checksum calculation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting.Azure/Provisioning/BicepUtilities.cs | Introduces skipKnownValues parameter to SetParametersAsync and uses it in GetCurrentChecksumAsync to prevent overwriting known parameters with null values |
| tests/Aspire.Hosting.Azure.Tests/BicepUtilitiesTests.cs | Adds test to verify known parameters are not overwritten when calculating checksums, ensuring roles resources don't redeploy unnecessarily |
| [AzureBicepResource.KnownParameters.PrincipalType] = "User", | ||
| [AzureBicepResource.KnownParameters.PrincipalId] = "1234" |
There was a problem hiding this comment.
The test creates a parameters JsonObject with direct values, but based on the ARM template parameter format used by SetParametersAsync and expected by GetChecksum, each parameter should be wrapped in a JsonObject with a "value" property.
The parameters should be structured as:
var parameters = new JsonObject
{
[AzureBicepResource.KnownParameters.PrincipalType] = new JsonObject { ["value"] = "User" },
[AzureBicepResource.KnownParameters.PrincipalId] = new JsonObject { ["value"] = "1234" }
};This matches the structure that SetParametersAsync creates (line 45-59 in BicepUtilities.cs) and what GetCurrentChecksumAsync expects when it parses parameters from configuration.
| [AzureBicepResource.KnownParameters.PrincipalType] = "User", | |
| [AzureBicepResource.KnownParameters.PrincipalId] = "1234" | |
| [AzureBicepResource.KnownParameters.PrincipalType] = new JsonObject { ["value"] = "User" }, | |
| [AzureBicepResource.KnownParameters.PrincipalId] = new JsonObject { ["value"] = "1234" } |
| /// This is important because if these known parameters are overwritten, it means the "roles" | ||
| /// resources will be redeployed every time the app is run. | ||
| /// </summary> | ||
| /// <returns></returns> |
There was a problem hiding this comment.
The return type documentation <returns></returns> is empty and should be removed since this is a Task returning method (equivalent to void). According to the XML documentation guidelines, <returns> tags are only required for methods that return a value (non-void returns).
|
Backport to 13.0 |
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/19275080058 |
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/19276695129 |
d3fce4b
into
microsoft:main
Description
Everytime we check if we need to redeploy an Azure "roles" resource in run mode, we are getting a different CheckSum. This is because we are overwriting the "known parameters" like PrincipalId and PrincipalType with 'null' values, because these values aren't available yet.
The fix is to skip setting those known parameters, like we did in previous versions.
Fix #12651
Checklist