Fix some references to LifecycleHooks that were actually Event Subscr…#15053
Fix some references to LifecycleHooks that were actually Event Subscr…#15053afscrome wants to merge 1 commit intomicrosoft:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15053Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15053" |
There was a problem hiding this comment.
Pull request overview
Aligns internal eventing subscriber naming in Aspire.Hosting (and associated tests) by replacing outdated *LifecycleHook* identifiers with *EventingSubscriber* / EventHandlers terminology, matching the current eventing model.
Changes:
- Renamed internal required-command validation subscriber and updated DI registration + test assertions.
- Renamed devcontainer port-forwarding subscriber and updated DI registration + test assertions.
- Renamed dashboard test class to match
DashboardEventHandlers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/RequiredCommandAnnotationTests.cs | Updates test names/assertions and doc comment to refer to RequiredCommandValidationEventingSubscriber. |
| tests/Aspire.Hosting.Tests/DistributedApplicationBuilderTests.cs | Updates default-service assertions to the renamed subscriber types. |
| tests/Aspire.Hosting.Tests/Dashboard/DashboardEventHandlersTests.cs | Renames the test class from DashboardLifecycleHookTests to DashboardEventHandlersTests. |
| src/Aspire.Hosting/Lifecycle/RequiredCommandValidationEventingSubscriber.cs | Renames the internal subscriber type to RequiredCommandValidationEventingSubscriber. |
| src/Aspire.Hosting/DistributedApplicationBuilder.cs | Updates eventing subscriber registrations to the renamed types. |
| src/Aspire.Hosting/Devcontainers/DevcontainerPortForwardingEventingSubscriber.cs | Renames the devcontainer eventing subscriber type and constructor. |
Comments suppressed due to low confidence (1)
tests/Aspire.Hosting.Tests/Dashboard/DashboardEventHandlersTests.cs:25
- This test file now uses
DashboardEventHandlersTests, but helper/variable names still use the older "hook" terminology (e.g.,CreateHook,hook). Consider renaming those to matchDashboardEventHandlersto keep naming consistent within the test suite.
| /// <summary> | ||
| /// Helper method to subscribe all eventing subscribers (including RequiredCommandValidationLifecycleHook) | ||
| /// Helper method to subscribe all eventing subscribers (including RequiredCommandValidationEventingSubscriber) | ||
| /// to the eventing system. This simulates what happens during app.StartAsync(). | ||
| /// </summary> | ||
| private static async Task SubscribeHooksAsync(DistributedApplication app) |
There was a problem hiding this comment.
The XML doc comment now describes subscribing eventing subscribers, but the helper is still named SubscribeHooksAsync. Renaming the method (and its call sites) to something like SubscribeEventingSubscribersAsync would keep terminology consistent and avoid confusion with lifecycle hooks.
2a55125 to
04fccae
Compare
Description
I noticed a few event subscribers still referred to
LifecycleHookin their class / method names. Renamed those it was safe to do. (Tests / internal classes)Checklist