Add non-localhost URLs #10232
Add non-localhost URLs #10232
Conversation
github-actions
bot
added
the
needs-area-label
BrennanConroy
force-pushed
the
damianedwards/non-localhost-urls
branch
from
222cddc to
039f225
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds support for generating and testing URLs for *.localhost subdomains and hosts that bind to multiple addresses.
- Added tests to verify URL annotations include both
localhostand custom.localhostdomains, and machine-name URLs for non-localhost hosts. - Updated
ApplicationOrchestratorto emit extraResourceUrlAnnotationentries when an endpoint is bound to multiple addresses or a.localhostsubdomain. - Enhanced
DcpExecutor.NormalizeTargetHostto treat any*.localhosthost aslocalhostwith a single-address binding.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/WithEndpointTests.cs | New tests validating URL annotations for .localhost domains and multi-address hosts. |
| src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs | Adds logic to append extra URLs for multi-address bindings and .localhost subdomains. |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Normalizes *.localhost hosts to localhost with single-address binding. |
Comments suppressed due to low confidence (1)
tests/Aspire.Hosting.Tests/WithEndpointTests.cs:652
- The tests for non-localhost hosts currently cover IPv4 (
0.0.0.0) but the IPv6 case is commented out. Consider adding a reliable assertion or skipping strategy for IPv6 (::) to ensure coverage across both IP versions.
[Theory]
| var endpointReference = new EndpointReference(resourceWithEndpoints, endpoint); | ||
| var url = new ResourceUrlAnnotation { Url = allocatedEndpoint.UriString, Endpoint = endpointReference }; | ||
| urls.Add(url); | ||
| if (allocatedEndpoint.BindingMode != EndpointBindingMode.SingleAddress && (endpoint.TargetHost is not "localhost" or "127.0.0.1")) |
There was a problem hiding this comment.
The check on endpoint.TargetHost.Length elsewhere uses a magic number (10) to detect .localhost domains. Rather than relying on string length, consider using EndsWith(".localhost", StringComparison.OrdinalIgnoreCase) by itself or extracting the suffix into a named constant for clarity.
Copilot uses AI. Check for mistakes.
| { | ||
| null or "" => ("localhost", EndpointBindingMode.SingleAddress), // Default is localhost | ||
| var s when string.Equals(s, "localhost", StringComparison.OrdinalIgnoreCase) => ("localhost", EndpointBindingMode.SingleAddress), // Explicitly set to localhost | ||
| var s when s.Length > 10 && s.EndsWith(".localhost", StringComparison.OrdinalIgnoreCase) => ("localhost", EndpointBindingMode.SingleAddress), // Explicitly set to localhost when using .localhost subdomain |
There was a problem hiding this comment.
The length check (s.Length > 10) is redundant because EndsWith(".localhost") already implies a minimum length. Consider removing the length condition or defining a clear constant for the subdomain suffix to improve readability.
| var s when s.Length > 10 && s.EndsWith(".localhost", StringComparison.OrdinalIgnoreCase) => ("localhost", EndpointBindingMode.SingleAddress), // Explicitly set to localhost when using .localhost subdomain | |
| var s when s.EndsWith(".localhost", StringComparison.OrdinalIgnoreCase) => ("localhost", EndpointBindingMode.SingleAddress), // Explicitly set to localhost when using .localhost subdomain |
Copilot uses AI. Check for mistakes.
|
/backport to release/9.4 |
|
Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16277041001 |
3 participants
Description
Adds urls for
*.localhostand other localhost adjacent bindings.Checklist