-
Notifications
You must be signed in to change notification settings - Fork 134
Remove deprecated IDistributedApplicationLifecycleHook interface #915
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
Remove deprecated IDistributedApplicationLifecycleHook interface #915
Conversation
Co-authored-by: aaronpowell <[email protected]>
Co-authored-by: aaronpowell <[email protected]>
|
You want to replace these with |
don't we want to replace them with the |
|
|
|
@copilot according to @davidfowl we should be using |
Co-authored-by: aaronpowell <[email protected]>
Updated Dapr to use Commit: 69ebc86 |
|
@davidfowl how's that looking? |
| // Only install packages during development, not in publish mode | ||
| if (!resource.ApplicationBuilder.ExecutionContext.IsPublishMode) | ||
| { | ||
| var installerName = $"{resource.Resource.Name}-bun-install"; |
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.
This call isn't idempotent. There's a new helper here https://github.com/dotnet/aspire/blob/5a87acfdb041ce3f89189a8af379fa24df4f3d44/src/Aspire.Hosting/DistributedApplicationBuilderExtensions.cs#L91.
You can use it like so https://github.com/dotnet/aspire/blob/5a87acfdb041ce3f89189a8af379fa24df4f3d44/src/Aspire.Hosting/ProjectResourceBuilderExtensions.cs#L822-L829
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.
But this is moving away from the event model for doing this to the same model used for npm/pnpm/etc. installing.
| if (!resource.ApplicationBuilder.ExecutionContext.IsPublishMode) | ||
| { | ||
| var installerName = $"{resource.Resource.Name}-deno-install"; | ||
| var installer = new DenoInstallerResource(installerName, resource.Resource.WorkingDirectory); |
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.
Same comment as above.
|
Yep! |
* baseline branch for the next major release * Updating nightly * Remove deprecated IDistributedApplicationLifecycleHook interface (#915) * Initial plan * Replace IDistributedApplicationLifecycleHook with event-based patterns Co-authored-by: aaronpowell <[email protected]> * Add tests for installer resources and update Dapr tests Co-authored-by: aaronpowell <[email protected]> * Refactor Dapr to use IDistributedApplicationEventingSubscriber Co-authored-by: aaronpowell <[email protected]> * Fixing build issue * Updating the endpoint port * Disabling some tests so we can move forward in other parts of the Aspire 13 update --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: aaronpowell <[email protected]> Co-authored-by: Aaron Powell <[email protected]> * Adjusting the version of keycloak * Updating aspire nightly * New nightly * Remove npm functionality moved to Aspire 13 (#928) * Initial plan * Remove AddViteApp and npm-specific functionality (moved to Aspire 13) Co-authored-by: aaronpowell <[email protected]> * Update comments and tests to reflect npm removal Co-authored-by: aaronpowell <[email protected]> * Update MONOREPO.md to remove npm references Co-authored-by: aaronpowell <[email protected]> * Adding missing types * Bad copilot code * Fixing missing namespace * Handling port forwarding * removing more stuff that has been migrated to aspire core * Reworking how package installing is handled * daily update * Adding back the annotation that is no longer moving to core * Forgot to install packages * adding a bunch more annotations to provide enough metadata * Removing old demo apps * Disabling python tests * Removing a testing change * Updating to the latest nightly and dealing with type name changes * Removing type that was can repurpose from aspire core * breaking the tests down so they are easier to read * Removing legacy docs * Fixing failing tests --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: aaronpowell <[email protected]> Co-authored-by: Aaron Powell <[email protected]> * Fixing failing test * Remove deprecated EventStore integrations (#946) * Initial plan * Remove EventStore integrations and all associated files Co-authored-by: aaronpowell <[email protected]> * Remove EventStore project references from solution file Co-authored-by: aaronpowell <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: aaronpowell <[email protected]> Co-authored-by: Aaron Powell <[email protected]> Co-authored-by: Fredi Machado <[email protected]> * Creating a new agent for generating hosting integrations * Creating a new agent for generating client integrations * Expanded the hosting agent a bit * Removing the legacy approach to checking resource health and updated the last usages of it * Bumping go and removing pnpm setup * maybe we need an explicit health check * Using the new JavaScriptAppResource so we can leverage the new methods on inspector Getting rid of some custom code but making it so we run from local by default * Disabling tests on Windows due to modelcontextprotocol/inspector#893 --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: aaronpowell <[email protected]> Co-authored-by: Fredi Machado <[email protected]>
Overview
This PR removes all usages of the deprecated
IDistributedApplicationLifecycleHookinterface and replaces them with modern event-based patterns, as required by dotnet/aspire#11266.Changes
Deno & Bun: Installer Resource Pattern
Previously, Deno and Bun used lifecycle hooks to install packages before application startup. This PR migrates them to use the installer resource pattern (consistent with NodeJS npm/yarn/pnpm):
Before:
After:
Files Changed:
DenoInstallerResource.cs,BunInstallerResource.csDenoPackageInstallerLifecycleHook.cs,DenoPackageInstaller.cs,BunPackageInstallerLifecycleHook.csDenoAppHostingExtensions.cs,BunAppExtensions.csDapr: IDistributedApplicationEventingSubscriber Pattern
Dapr's lifecycle hook has been converted to use the
IDistributedApplicationEventingSubscriberpattern for idempotent registration:Before:
After:
Files Changed:
IDistributedApplicationBuilderExtensions.cs,DaprDistributedApplicationLifecycleHook.csPattern Benefits:
TryAddEventingSubscriber(similar to oldTryAddLifecycleHook)Tests
Updated tests to use standard public APIs instead of internal lifecycle hook methods:
ExecuteBeforeStartHooksAsync()withapp.StartAsync()UnsafeAccessorreflection codeBenefits
IDistributedApplicationLifecycleHookIDistributedApplicationEventingSubscriberfor proper lifecycle managementMigration Notes
No breaking changes for users:
WithDenoPackageInstallation()andWithBunPackageInstallation()work exactly as beforeAddDapr()continues to work with the same APIFixes #890
Original prompt
Fixes #890
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.