-
Notifications
You must be signed in to change notification settings - Fork 685
Address feedback to WithUrls() #8602
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
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.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
tests/Aspire.Hosting.Tests/WithUrlsTests.cs:47
- [nitpick] The test method name 'WithUrlsCallsCallbackAfterBeforeResourceStartedEvent' is ambiguous due to the repeated 'AfterBefore' phrasing. Consider renaming it to 'WithUrlsCallsCallbackAfterResourceStartedEvent' for clarity.
public async Task WithUrlsCallsCallbackAfterBeforeResourceStartedEvent()
src/Aspire.Hosting/Dcp/ResourceSnapshotBuilder.cs:213
- The use of the null-forgiving operator on 'endpointUrl.Endpoint' assumes this property is never null. Consider adding a null-check or ensuring via code invariants that 'endpointUrl.Endpoint' is always set to avoid potential runtime exceptions.
urls.Add(new(Name: endpointUrl.Endpoint!.EndpointName, Url: endpointUrl.Url, IsInternal: false) { IsInactive = isInactive, DisplayProperties = new(endpointUrl.DisplayText ?? "", endpointUrl.DisplayOrder ?? 0) });
/backport to release/9.2 |
Started backporting to release/9.2: https://github.com/dotnet/aspire/actions/runs/14321482119 |
} | ||
|
||
private async Task OnResourceStarting(OnResourceStartingContext context) | ||
{ | ||
// Call the callbacks to configure resource URLs | ||
await ProcessUrls(context.Resource, context.CancellationToken).ConfigureAwait(false); |
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.
What happens if this fails?
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.
Good question given it's user code. This didn't change in this PR of course, but there's no error handling here right now. I see that the environment callback stuff doesn't handle errors in its callbacks either though.
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.
So same experience, failed to start
Description
Addresses feedback received about using
WithUrls()
:BeforeResourceStarted
event processingWithUrl
that acceptReferenceExpression
and interpolated stringFixes #8587
Checklist
<remarks />
and<code />
elements on your triple slash comments?