Migrate DotnetToolResource to use RequiredCommandValidator#15265
Closed
afscrome wants to merge 1 commit intomicrosoft:release/13.2from
Closed
Migrate DotnetToolResource to use RequiredCommandValidator#15265afscrome wants to merge 1 commit intomicrosoft:release/13.2from
DotnetToolResource to use RequiredCommandValidator#15265afscrome wants to merge 1 commit intomicrosoft:release/13.2from
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15265Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15265" |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates DotnetToolResource’s .NET SDK version check to the shared RequiredCommandValidator flow and adjusts the validator’s caching behavior to account for different validation callbacks per command.
Changes:
- Add a
WithRequiredCommand("dotnet", ...)validation toDotnetToolResourceand remove the prior before-start SDK version check. - Update
RequiredCommandValidatorcaching to key by(command, validationCallback)instead of justcommand. - Update/add tests for required-command caching behavior and for
AddDotnetToolemitting the 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 across identical vs different validation callbacks. |
| tests/Aspire.Hosting.DotnetTool.Tests/AddDotnetToolTests.cs | Removes direct SDK version validation unit tests; adds an assertion that AddDotnetTool includes a RequiredCommandAnnotation. |
| src/Aspire.Hosting/DotnetToolResourceExtensions.cs | Moves SDK version validation into a required-command validation callback and removes the prior before-start validation call. |
| src/Aspire.Hosting/ApplicationModel/RequiredCommandValidator.cs | Changes validation caching key to include the callback and introduces a cache-key type. |
Comments suppressed due to low confidence (1)
src/Aspire.Hosting/ApplicationModel/RequiredCommandValidator.cs:106
- Including
ValidationCallbackin the cache key means missing-command failures (whereresolvedis null and the callback is never invoked) will no longer be coalesced across resources if they supply different callbacks for the same command. That can produce multiple identical warnings/notifications for a single missing executable. Consider splitting caching so command resolution/notification is keyed only by command, while callback execution results are cached separately (e.g., per-command state that tracks per-callback validation).
// Get or create state for this command/callback combination.
var cacheKey = new CommandValidationCacheKey(command, annotation.ValidationCallback);
var state = _commandStates.GetOrAdd(cacheKey, _ => new CommandValidationState());
await state.Gate.WaitAsync(cancellationToken).ConfigureAwait(false);
try
{
// If validation already failed for this command, just log and return the cached failure
if (state.ErrorMessage is not null)
{
_logger.LogWarning("Resource '{ResourceName}' may fail to start: {Message}", resource.Name, state.ErrorMessage);
return RequiredCommandValidationResult.Failure(state.ErrorMessage);
}
// Check if already validated successfully
if (state.ResolvedPath is not null)
{
_logger.LogDebug("Required command '{Command}' for resource '{ResourceName}' already validated, resolved to '{ResolvedPath}'.", command, resource.Name, state.ResolvedPath);
return RequiredCommandValidationResult.Success();
}
// Perform validation
var resolved = ResolveCommand(command);
var isValid = true;
string? validationMessage = null;
if (resolved is not null && annotation.ValidationCallback is not null)
{
var context = new RequiredCommandValidationContext(resolved, _serviceProvider, cancellationToken);
var result = await annotation.ValidationCallback(context).ConfigureAwait(false);
isValid = result.IsValid;
validationMessage = result.ValidationMessage;
}
if (resolved is null || !isValid)
{
var link = annotation.HelpLink;
// Build the message for logging and exceptions (includes inline link if available)
var message = (link, validationMessage) switch
{
(null, not null) => validationMessage,
(not null, not null) => string.Format(CultureInfo.CurrentCulture, MessageStrings.RequiredCommandValidationFailedWithLink, command, validationMessage, link),
(not null, null) => string.Format(CultureInfo.CurrentCulture, MessageStrings.RequiredCommandNotFoundWithLink, command, link),
_ => string.Format(CultureInfo.CurrentCulture, MessageStrings.RequiredCommandNotFound, command)
Comment on lines
24
to
65
| @@ -59,8 +59,9 @@ public async Task<RequiredCommandValidationResult> ValidateAsync( | |||
| throw new InvalidOperationException($"Required command on resource '{resource.Name}' cannot be null or empty."); | |||
| } | |||
|
|
|||
| // Get or create state for this command | |||
| var state = _commandStates.GetOrAdd(command, _ => new CommandValidationState()); | |||
| // Get or create state for this command/callback combination. | |||
| var cacheKey = new CommandValidationCacheKey(command, annotation.ValidationCallback); | |||
| var state = _commandStates.GetOrAdd(cacheKey, _ => new CommandValidationState()); | |||
|
|
|||
Contributor
Author
There was a problem hiding this comment.
Should the command name have been case insensitive in the first place? Yes on windows commands are case insensitive, but that's not true for linux/mac.
Comment on lines
+205
to
+214
| internal static async Task<RequiredCommandValidationResult> ValidateDotnetSdkVersionAsync(RequiredCommandValidationContext _, string workingDirectory) | ||
| { | ||
| if (version is null) | ||
| { | ||
| // This most likely means something is majorly wrong with the dotnet sdk | ||
| // Which will show up as an error once dcp runs `dotnet tool exec` | ||
| return; | ||
| } | ||
| var version = await DotnetSdkUtils.TryGetVersionAsync(workingDirectory).ConfigureAwait(false); | ||
|
|
||
| if (version.Major < 10) | ||
| if (version?.Major < 10) | ||
| { | ||
| throw new DistributedApplicationException($"DotnetToolResource requires dotnet SDK 10 or higher to run. Detected version: {version} for working directory {workingDirectory}"); | ||
| return RequiredCommandValidationResult.Failure($"DotnetToolResource requires dotnet SDK 10 or higher to run. Detected version: {version}"); | ||
| } | ||
|
|
||
| return RequiredCommandValidationResult.Success(); |
Comment on lines
+186
to
+190
| 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; |
- 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.
1cdb197 to
27010fc
Compare
5 tasks
This file contains hidden or 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
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.
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
Checklist