Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14287Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14287" |
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where dashboard URLs displayed "localhost" instead of the configured localhost TLD (e.g., "aspire-dashboard.dev.localhost") when using the --localhost-tld template option. The issue affected the Aspire 13.2 project template.
Changes:
- Introduced
EndpointHostHelpers.GetUrlWithTargetHostAsync()helper method to resolve endpoint URLs with the configured TLD hostname when applicable - Updated dashboard URL logging in
DashboardEventHandlers.csto use the new helper method - Updated API base URL resolution in
DashboardUrlsHelper.csto use the new helper method - Expanded test coverage to verify correct URL resolution for multiple scenarios including localhost TLDs
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting/ApplicationModel/EndpointHostHelpers.cs | Added internal helper method GetUrlWithTargetHostAsync() to adjust endpoint URLs for localhost TLD configuration |
| src/Aspire.Hosting/Dashboard/DashboardEventHandlers.cs | Updated dashboard URL logging to use the new helper method instead of direct GetValueAsync() |
| src/Aspire.Hosting/Backchannel/DashboardUrlsHelper.cs | Updated API base URL resolution to use the new helper method |
| tests/Aspire.Hosting.Tests/Dashboard/DashboardLifecycleHookTests.cs | Converted test to theory with multiple test cases covering localhost, localhost TLDs, and different schemes |
| var uri = new Uri(allocatedUrl); | ||
| return $"{uri.Scheme}://{targetHost}:{uri.Port}"; |
There was a problem hiding this comment.
The Uri constructor on line 138 could throw an exception if allocatedUrl is not a valid absolute URI. While the check on line 127 ensures it's not null or empty, it doesn't validate that it's a well-formed URI. Consider adding a try-catch block or using Uri.TryCreate to handle potential format exceptions gracefully, especially since this is an internal utility method that may be called from various contexts.
| var uri = new Uri(allocatedUrl); | |
| return $"{uri.Scheme}://{targetHost}:{uri.Port}"; | |
| if (Uri.TryCreate(allocatedUrl, UriKind.Absolute, out var uri)) | |
| { | |
| return $"{uri.Scheme}://{targetHost}:{uri.Port}"; | |
| } | |
| // If the allocated URL is not a valid absolute URI, return it unchanged. | |
| return allocatedUrl; |
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #21586229647 |
|
There is probably a room for improvement for DCP here as well--opened microsoft/dcp#68 to pursue that. |
Description
Fixes #14248
The value returned from
EndpointReferenceis the correct port, but has alocalhosthost. It doesn't reflect the target host value.Update places where GetValueAsync is used to resolve dashboard URLs with a helper method that corrects the host.
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate