Migrate DotnetToolResource to use RequiredCommandValidator#15267
Migrate DotnetToolResource to use RequiredCommandValidator#15267afscrome wants to merge 2 commits intomicrosoft:mainfrom
DotnetToolResource to use RequiredCommandValidator#15267Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15267Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15267" |
dotnet-policy-service
bot
added
the
community-contribution
There was a problem hiding this comment.
Pull request overview
This PR migrates DotnetToolResource’s .NET SDK version validation to run through the shared RequiredCommandValidator, and adjusts the validator’s caching so different validation callbacks don’t incorrectly share cached results (addressing #14304).
Changes:
DotnetToolResourceExtensionsnow registers aWithRequiredCommand("dotnet", ...)callback instead of performing the SDK check inOnBeforeResourceStarted.RequiredCommandValidatorcache key is expanded to include the validation callback (command + callback).- Tests updated to validate caching behavior across callbacks and that dotnet tool resources emit a required-command annotation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/RequiredCommandAnnotationTests.cs | Adds coverage for caching behavior when callbacks are the same vs different. |
| tests/Aspire.Hosting.DotnetTool.Tests/AddDotnetToolTests.cs | Replaces direct SDK version validation tests with an annotation presence test. |
| src/Aspire.Hosting/DotnetToolResourceExtensions.cs | Moves SDK version validation into a WithRequiredCommand validation callback. |
| src/Aspire.Hosting/ApplicationModel/RequiredCommandValidator.cs | Changes cache key to include the callback to avoid cross-callback caching. |
| private readonly record struct CommandValidationCacheKey(string command, Func<RequiredCommandValidationContext, Task<RequiredCommandValidationResult>>? callback) | ||
| { | ||
| public string Command { get; } = command; | ||
|
|
||
| public Func<RequiredCommandValidationContext, Task<RequiredCommandValidationResult>>? Callback { get; } = callback; | ||
| } | ||
|
|
| [Fact] | ||
| public void ValidateDotnetSdkVersion_WithNullVersion_DoesNotThrow() | ||
| public void AddDotnetTool_IncludesRequiredCommandAnnotation() | ||
| { | ||
| // Should not throw - null is treated as "unable to determine version" | ||
| DotnetToolResourceExtensions.ValidateDotnetSdkVersion(null, ""); | ||
| var builder = DistributedApplication.CreateBuilder(); | ||
| var tool = builder.AddDotnetTool("mytool", "dotnet-ef"); |
| // Track validation state per command/callback pair to coalesce notifications and validation work. | ||
| private readonly ConcurrentDictionary<CommandValidationCacheKey, CommandValidationState> _commandStates = new(); |
There was a problem hiding this comment.
For linux & mac, I think case insensitive is actually correct here as dotnet and DOTNET are different executables.
afscrome
force-pushed
the
dotnettool-requiredcommand
branch
from
27010fc to
118c896
Compare
|
I don't think this is critical for 13.2. @afscrome - can you retarget this to |
|
Thanks a bunch for the change @afscrome! Given 13.2.0 is about to get released, we've bumped the bug bar criteria this week, and unfortunately that means it is a bit late to take this in for next week. As @eerhardt suggests, can we retarget this to main and we can backport it to the release branch after next week if we think this is something we should service in 13.2.1? Thanks again and sorry for the trouble! |
afscrome
requested review from
JamesNK,
danegsta,
davidfowl and
karolz-ms
as code owners
- Migrated `dotnet tool` version validation to use `RequiredCommandValidator` - Updated `RequiredCommandValidator` to include the callback in teh cache key, so that dotnet tools resources with different working directories are evaluated separately.
afscrome
force-pushed
the
dotnettool-requiredcommand
branch
from
809b58d to
0138079
Compare
Description
dotnet toolversion validation to useRequiredCommandValidatorRequiredCommandValidatorto include the callback in the cache key, so that resources for the same tool name, but different validation callbacks have the correct callback executed.Fixes #14304. This is a second attempt of #15265 - changing the base branch seemed to confuse the test automation.
Checklist