-
Notifications
You must be signed in to change notification settings - Fork 689
Obsolete the IDistributedApplicationLifecycleHook
interface and extension methods and add a new eventing based replacement
#11266
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
base: main
Are you sure you want to change the base?
Conversation
🚀 Dogfood this PR with: curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11266 Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11266" |
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
This PR deprecates the IDistributedApplicationLifecycleHook
interface and its extension methods, replacing them with a new event-driven approach using IDistributedApplicationEventingSubscriber
. This change is necessary to properly support asynchronous endpoint discovery for new features like proxyless persistent containers with random port allocation.
- Marks the existing lifecycle hook interface and extension methods as obsolete
- Introduces new
IDistributedApplicationEventingSubscriber
interface and corresponding extension methods - Migrates all internal usage from lifecycle hooks to the new eventing system
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Aspire.Hosting/Lifecycle/IDistributedApplicationLifecycleHook.cs | Marked interface as obsolete |
src/Aspire.Hosting/Lifecycle/LifecycleHookServiceCollectionExtensions.cs | Marked extension methods as obsolete |
src/Aspire.Hosting/Lifecycle/IDistributedApplicationEventingSubscriber.cs | New eventing subscriber interface |
src/Aspire.Hosting/Lifecycle/EventingSubscriberServiceCollectionExtensions.cs | New extension methods for eventing subscribers |
src/Aspire.Hosting/Dashboard/DashboardEventHandlers.cs | Migrated from lifecycle hook to eventing subscriber pattern |
src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingLifecycleHook.cs | Migrated to eventing subscriber while keeping class name |
src/Aspire.Hosting/DistributedApplication.cs | Updated to initialize eventing subscribers before publishing events |
Multiple Azure infrastructure files | Migrated Azure provisioning and infrastructure classes to use eventing |
Multiple test files | Added pragma warnings to suppress obsolete API usage in tests |
Multiple playground files | Updated example projects to use new eventing pattern |
src/Aspire.Hosting.Azure/Provisioning/Provisioners/AzureProvisioner.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppsInfrastructure.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/Lifecycle/IDistributedApplicationEventingSubscriber.cs
Outdated
Show resolved
Hide resolved
I think we need to stage this. The eventing has a bunch of issues that prevent it from being a full replacement at the moment: |
/// <param name="executionContext">The <see cref="DistributedApplicationExecutionContext"/> instance for the run.</param> | ||
/// <param name="cancellationToken">Cancellation token from the service collection</param> | ||
/// <returns>A task indicating event registration is complete</returns> | ||
Task SubscribeAsync(IDistributedApplicationEventing eventing, DistributedApplicationExecutionContext executionContext, CancellationToken cancellationToken); |
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.
Is the expected pattern for conditional subscription to do the check within SubscribeAsync
? I assume so, but this will be a bit different from how we currently do conditional subscription. Maybe we can add a test to validate/document this?
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.
That was the pattern that seemed to make the most sense. With the callback based model there wasn't a way to selectively subscribe to a given hook so we did the checks in the actual callback, but with events it seemed better to only subscribe to the event if we actually wanted to receive it.
Ah, I hadn't seen those issues, but ended up adding a new |
IDistirbutedApplicationLifecycleHook
interface and extension methods and add a new eventing based replacementIDistributedApplicationLifecycleHook
interface and extension methods and add a new eventing based replacement
Description
Deprecates
IDistributedApplicationLifecycleHook
and theLifecycleHookServiceCollectionExtensions
extension methods and adds a newIDistributedApplicationEventingSubscriber
andEventingSubscriberServiceCollectionExtensions
methods to allow services to use the equivalentIDistributedApplicationEventing
events (includingBeforeStartEvent
).This change is necessary to complete the removal of the old
AfterEndpointsAllocated
event and callback in a future release which will allow us to properly support new features that require asynchronous discovery of allocated endpoints (such as proxyless persistent containers with random port allocation). These obsolete types will be removed in a future release, but the replacement has been designed to be relatively painless.Checklist
<remarks />
and<code />
elements on your triple slash comments?doc-idea
templatebreaking-change
templatediagnostic
template