Skip to content

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 8, 2025
Merged

Address feedback to WithUrls() #8602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
102d1f8
Address WithUrls feedback
DamianEdwards Apr 7, 2025
8d8844d
Add more WithUrl tests
DamianEdwards Apr 7, 2025
31da6d7
More tests
DamianEdwards Apr 7, 2025
af51a96
Fix test failure
DamianEdwards Apr 7, 2025
a26964a
Fix test
DamianEdwards Apr 7, 2025
4fa2d9d
PR feedback
DamianEdwards Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/Aspire.Hosting/Dcp/ResourceSnapshotBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public ResourceSnapshotBuilder(DcpResourceState resourceState)
public CustomResourceSnapshot ToSnapshot(Container container, CustomResourceSnapshot previous)
{
var containerId = container.Status?.ContainerId;
var urls = GetUrls(container);
var urls = GetUrls(container, container.Status?.State);
var volumes = GetVolumes(container);

var environment = GetEnvironmentVariables(container.Status?.EffectiveEnv ?? container.Spec.Env, container.Spec.Env);
Expand Down Expand Up @@ -99,7 +99,7 @@ public CustomResourceSnapshot ToSnapshot(Executable executable, CustomResourceSn

var state = executable.AppModelInitialState is "Hidden" ? "Hidden" : executable.Status?.State;

var urls = GetUrls(executable);
var urls = GetUrls(executable, executable.Status?.State);

var environment = GetEnvironmentVariables(executable.Status?.EffectiveEnv, executable.Spec.Env);

Expand Down Expand Up @@ -183,7 +183,7 @@ private static (ImmutableArray<string> Args, ImmutableArray<int>? ArgsAreSensiti
return (launchArgsBuilder.ToImmutable(), argsAreSensitiveBuilder.ToImmutable(), anySensitive);
}

private ImmutableArray<UrlSnapshot> GetUrls(CustomResource resource)
private ImmutableArray<UrlSnapshot> GetUrls(CustomResource resource, string? resourceState)
{
var urls = ImmutableArray.CreateBuilder<UrlSnapshot>();
var appModelResourceName = resource.AppModelResourceName;
Expand All @@ -199,21 +199,26 @@ private ImmutableArray<UrlSnapshot> GetUrls(CustomResource resource)
var name = resource.Metadata.Name;

// Add the endpoint URLs
foreach (var service in resourceServices)
var serviceEndpoints = new HashSet<(string EndpointName, string ServiceMetadataName)>(resourceServices.Where(s => !string.IsNullOrEmpty(s.EndpointName)).Select(s => (s.EndpointName!, s.Metadata.Name)));
foreach (var endpoint in serviceEndpoints)
{
if (endpointUrls.FirstOrDefault(u => string.Equals(service.EndpointName, u.Endpoint?.EndpointName, StringComparisons.EndpointAnnotationName)) is { Endpoint: { } } endpointUrl)
var (endpointName, serviceName) = endpoint;
var urlsForEndpoint = endpointUrls.Where(u => string.Equals(endpointName, u.Endpoint?.EndpointName, StringComparisons.EndpointAnnotationName)).ToList();

foreach (var endpointUrl in urlsForEndpoint)
{
var activeEndpoint = _resourceState.EndpointsMap.SingleOrDefault(e => e.Value.Spec.ServiceName == service.Metadata.Name && e.Value.Metadata.OwnerReferences?.Any(or => or.Kind == resource.Kind && or.Name == name) == true).Value;
var activeEndpoint = _resourceState.EndpointsMap.SingleOrDefault(e => e.Value.Spec.ServiceName == serviceName && e.Value.Metadata.OwnerReferences?.Any(or => or.Kind == resource.Kind && or.Name == name) == true).Value;
var isInactive = activeEndpoint is null;

urls.Add(new(Name: endpointUrl.Endpoint.EndpointName, Url: endpointUrl.Url, IsInternal: false) { IsInactive = isInactive, DisplayProperties = new(endpointUrl.DisplayText ?? "", endpointUrl.DisplayOrder ?? 0) });
urls.Add(new(Name: endpointUrl.Endpoint!.EndpointName, Url: endpointUrl.Url, IsInternal: false) { IsInactive = isInactive, DisplayProperties = new(endpointUrl.DisplayText ?? "", endpointUrl.DisplayOrder ?? 0) });
}
}

// Add the non-endpoint URLs
var resourceRunning = string.Equals(resourceState, KnownResourceStates.Running, StringComparisons.ResourceState);
foreach (var url in nonEndpointUrls)
{
urls.Add(new(Name: null, Url: url.Url, IsInternal: false) { IsInactive = false, DisplayProperties = new(url.DisplayText ?? "", url.DisplayOrder ?? 0) });
urls.Add(new(Name: null, Url: url.Url, IsInternal: false) { IsInactive = !resourceRunning, DisplayProperties = new(url.DisplayText ?? "", url.DisplayOrder ?? 0) });
}
}

Expand Down
67 changes: 41 additions & 26 deletions src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,13 @@ private async Task OnEndpointsAllocated(OnEndpointsAllocatedContext context)
{
await lifecycleHook.AfterEndpointsAllocatedAsync(_model, context.CancellationToken).ConfigureAwait(false);
}

await ProcessUrls(context.CancellationToken).ConfigureAwait(false);
}

private async Task OnResourceStarting(OnResourceStartingContext context)
{
// Call the callbacks to configure resource URLs
await ProcessUrls(context.Resource, context.CancellationToken).ConfigureAwait(false);
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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


switch (context.ResourceType)
{
case KnownResourceTypes.Project:
Expand Down Expand Up @@ -152,44 +153,58 @@ private async Task OnResourcesPrepared(OnResourcesPreparedContext _)
await PublishResourcesWithInitialStateAsync().ConfigureAwait(false);
}

private async Task ProcessUrls(CancellationToken cancellationToken)
private async Task ProcessUrls(IResource resource, CancellationToken cancellationToken)
{
// Project endpoints to URLS
foreach (var resource in _model.Resources.OfType<IResourceWithEndpoints>())
if (resource is not IResourceWithEndpoints resourceWithEndpoints)
{
var urls = new List<ResourceUrlAnnotation>();
return;
}

if (resource.TryGetEndpoints(out var endpoints))
// Project endpoints to URLS
var urls = new List<ResourceUrlAnnotation>();

if (resource.TryGetEndpoints(out var endpoints))
{
foreach (var endpoint in endpoints)
{
foreach (var endpoint in endpoints)
// Create a URL for each endpoint
if (endpoint.AllocatedEndpoint is { } allocatedEndpoint)
{
// Create a URL for each endpoint
if (endpoint.AllocatedEndpoint is { } allocatedEndpoint)
{
var url = new ResourceUrlAnnotation { Url = allocatedEndpoint.UriString, Endpoint = new EndpointReference(resource, endpoint) };
urls.Add(url);
}
var url = new ResourceUrlAnnotation { Url = allocatedEndpoint.UriString, Endpoint = new EndpointReference(resourceWithEndpoints, endpoint) };
urls.Add(url);
}
}
}

// Run the URL callbacks
if (resource.TryGetAnnotationsOfType<ResourceUrlsCallbackAnnotation>(out var callbacks))
// Run the URL callbacks
if (resource.TryGetAnnotationsOfType<ResourceUrlsCallbackAnnotation>(out var callbacks))
{
var urlsCallbackContext = new ResourceUrlsCallbackContext(new(DistributedApplicationOperation.Run), resource, urls, cancellationToken)
{
var urlsCallbackContext = new ResourceUrlsCallbackContext(new(DistributedApplicationOperation.Run), resource, urls, cancellationToken)
{
Logger = _loggerService.GetLogger(resource.Name)
};
foreach (var callback in callbacks)
{
await callback.Callback(urlsCallbackContext).ConfigureAwait(false);
}
Logger = _loggerService.GetLogger(resource.Name)
};
foreach (var callback in callbacks)
{
await callback.Callback(urlsCallbackContext).ConfigureAwait(false);
}
}

foreach (var url in urls)
// Clear existing URLs
if (resource.TryGetUrls(out var existingUrls))
{
var existing = existingUrls.ToArray();
for (var i = existing.Length - 1; i >= 0; i--)
{
resource.Annotations.Add(url);
var url = existing[i];
resource.Annotations.Remove(url);
}
}

// Add URLs
foreach (var url in urls)
{
resource.Annotations.Add(url);
}
}

private Task ProcessResourcesWithoutLifetime(AfterEndpointsAllocatedEvent @event, CancellationToken cancellationToken)
Expand Down
53 changes: 52 additions & 1 deletion src/Aspire.Hosting/ResourceBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,58 @@ public static IResourceBuilder<T> WithUrl<T>(this IResourceBuilder<T> builder, s
}

/// <summary>
/// Registers a callback to customize the URL displayed for the endpoint with the specified name.
/// Adds a URL to be displayed for the resource.
/// </summary>
/// <typeparam name="T">The resource type.</typeparam>
/// <param name="builder">The builder for the resource.</param>
/// <param name="url">The interpolated string that produces the URL.</param>
/// <param name="displayText">The display text to show when the link is displayed.</param>
/// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
/// <remarks>
/// Use this method to add a URL to be displayed for the resource.<br/>
/// Note that any endpoints on the resource will automatically get a corresponding URL added for them.
/// </remarks>
public static IResourceBuilder<T> WithUrl<T>(this IResourceBuilder<T> builder, in ReferenceExpression.ExpressionInterpolatedStringHandler url, string? displayText = null)
where T : IResource
{
ArgumentNullException.ThrowIfNull(builder);

var expression = url.GetExpression();

return builder.WithUrl(expression, displayText);
}

/// <summary>
/// Adds a URL to be displayed for the resource.
/// </summary>
/// <typeparam name="T">The resource type.</typeparam>
/// <param name="builder">The builder for the resource.</param>
/// <param name="url">A <see cref="ReferenceExpression"/> that will produce the URL.</param>
/// <param name="displayText">The display text to show when the link is displayed.</param>
/// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
/// <remarks>
/// Use this method to add a URL to be displayed for the resource.<br/>
/// Note that any endpoints on the resource will automatically get a corresponding URL added for them.
/// </remarks>
public static IResourceBuilder<T> WithUrl<T>(this IResourceBuilder<T> builder, ReferenceExpression url, string? displayText = null)
where T : IResource
{
ArgumentNullException.ThrowIfNull(builder);
ArgumentNullException.ThrowIfNull(url);

return builder.WithAnnotation(new ResourceUrlsCallbackAnnotation(async c =>
{
var endpoint = url.ValueProviders.OfType<EndpointReference>().FirstOrDefault();
var urlValue = await url.GetValueAsync(c.CancellationToken).ConfigureAwait(false);
if (!string.IsNullOrEmpty(urlValue))
{
c.Urls.Add(new() { Endpoint = endpoint, Url = urlValue, DisplayText = displayText });
}
}));
}

/// <summary>
/// Registers a callback to update the URL displayed for the endpoint with the specified name.
/// </summary>
/// <typeparam name="T">The resource type.</typeparam>
/// <param name="builder">The builder for the resource.</param>
Expand Down
50 changes: 25 additions & 25 deletions tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,26 +177,26 @@ public async Task ExplicitStart_StartExecutable()
var notStartedResourceEvent = await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.NotStarted).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
var dependentResourceEvent = await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Waiting).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);

// Inactive URLs and source should be populated on non-started resources.
// Source should be populated on non-started resources.
Assert.Contains("TestProject.ServiceA.csproj", notStartedResourceEvent.Snapshot.Properties.Single(p => p.Name == "project.path").Value?.ToString());
Assert.Collection(notStartedResourceEvent.Snapshot.Urls, u =>
{
Assert.Equal("http://localhost:5156", u.Url);
Assert.True(u.IsInactive);
});
Assert.Contains("TestProject.ServiceB.csproj", dependentResourceEvent.Snapshot.Properties.Single(p => p.Name == "project.path").Value?.ToString());
Assert.Collection(dependentResourceEvent.Snapshot.Urls, u =>
{
Assert.Equal("http://localhost:5254", u.Url);
Assert.True(u.IsInactive);
});

logger.LogInformation("Start explicit start resource.");
await orchestrator.StartResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
var runningResourceEvent = await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
Assert.Collection(runningResourceEvent.Snapshot.Urls, u =>
{
Assert.Equal("http://localhost:5156", u.Url);
Assert.Equal("http", u.Name);
});

// Dependent resource should now run.
await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
var dependentResourceRunningEvent = await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
Assert.Collection(dependentResourceRunningEvent.Snapshot.Urls, u =>
{
Assert.Equal("http://localhost:5254", u.Url);
Assert.Equal("http", u.Name);
});

logger.LogInformation("Stop resource.");
await orchestrator.StopResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
Expand Down Expand Up @@ -239,27 +239,27 @@ public async Task ExplicitStart_StartContainer()
var notStartedResourceEvent = await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.NotStarted).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
var dependentResourceEvent = await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Waiting).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);

// Inactive URLs and source should be populated on non-started resources.
// Source should be populated on non-started resources.
Assert.Equal(RedisImageSource, notStartedResourceEvent.Snapshot.Properties.Single(p => p.Name == "container.image").Value?.ToString());
Assert.Collection(notStartedResourceEvent.Snapshot.Urls, u =>
Assert.Contains("TestProject.ServiceB.csproj", dependentResourceEvent.Snapshot.Properties.Single(p => p.Name == "project.path").Value?.ToString());

logger.LogInformation("Start explicit start resource.");
await orchestrator.StartResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
var runningResourceEvent = await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
Assert.Collection(runningResourceEvent.Snapshot.Urls, u =>
{
Assert.Equal("tcp://localhost:6379", u.Url);
Assert.True(u.IsInactive);
});
Assert.Contains("TestProject.ServiceB.csproj", dependentResourceEvent.Snapshot.Properties.Single(p => p.Name == "project.path").Value?.ToString());
Assert.Collection(dependentResourceEvent.Snapshot.Urls, u =>

// Dependent resource should now run.
var dependentRunningResourceEvent = await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
Assert.Collection(dependentRunningResourceEvent.Snapshot.Urls, u =>
{
Assert.Equal("http://localhost:5254", u.Url);
Assert.True(u.IsInactive);
Assert.Equal("http", u.Name);
});

logger.LogInformation("Start explicit start resource.");
await orchestrator.StartResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);

// Dependent resource should now run.
await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);

logger.LogInformation("Stop resource.");
await orchestrator.StopResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Exited).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
Expand Down
Loading
Loading