-
Notifications
You must be signed in to change notification settings - Fork 696
Match name with ResourceCommandService #10370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cc @afscrome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Implements fallback matching by resource name when no replicas exist and clarifies this behavior in XML documentation, plus adds corresponding unit tests.
- Adds fallback logic in
ResourceNotificationService
to match resources by name if only one exists. - Updates XML docs in both
ResourceNotificationService
andResourceCommandService
to explain resource ID vs. name matching. - Introduces tests covering both failure and success scenarios for resource name matching in
ResourceCommandService
.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
tests/Aspire.Hosting.Tests/ResourceCommandServiceTests.cs | Adds tests for resource name matching (failure and success) |
src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs | Implements name-based fallback lookup and updates XML docs |
src/Aspire.Hosting/ApplicationModel/ResourceCommandService.cs | Updates XML docs to clarify resourceId vs. resourceName logic |
Comments suppressed due to low confidence (2)
src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs:448
- Fix the grammar and spelling in this XML comment: change "can be also be used" to "can also be used" and correct "specifing" to "specifying".
/// </para>
src/Aspire.Hosting/ApplicationModel/ResourceCommandService.cs:37
- Fix the grammar and spelling in this XML comment: change "can be also be used" to "can also be used" and correct "specifing" to "specifying".
/// The resource name can be also be used to retrieve the resource state, but it must be unique. If there are multiple resources with the same name, then this method will not return a match.
src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
else | ||
{ | ||
// Second match found, so we can't return a match based on the name. | ||
nameMatch = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replicas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
/backport to release/9.4 |
Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16258280487 |
Description
ResourceCommandService.ExecuteCommandAsync(string, ...)
takes a resourceId and matches to the resource with that resourceId. That includes the suffix for DCP resources, e.g.cache-sfsdfsdf
. This could be confusing as people often only think about the resource name, e.g.cache
.PR improves XML docs for API. It also allows the method to match on resource name if there are no replicas.
Fixes #10224
Checklist
<remarks />
and<code />
elements on your triple slash comments?doc-idea
templatebreaking-change
templatediagnostic
template