-
Notifications
You must be signed in to change notification settings - Fork 668
Remove IResourceWithoutLifetime on everything #10851
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
- ParameterResources can now be waited on - Move init logic to ConnectionStringResource class and simplify it. Delete special logic for WaitFor ConnectionStringResource. - Changed state to Running for ParameterResource and ConnectionStringResource
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 removes the IResourceWithoutLifetime
interface from various resource types to enable waiting functionality on parameter and connection string resources. The change allows parameters and connection strings to participate in the normal resource lifecycle, making them awaitable through WaitFor
operations.
- Removes
IResourceWithoutLifetime
fromParameterResource
,ConnectionStringResource
, andExternalServiceResource
- Moves connection string initialization logic from the orchestrator to the
ConnectionStringResource
class - Updates resource states to use
Running
instead ofActive
for parameter and connection string resources
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/Aspire.Hosting.Tests/ExternalServiceTests.cs |
Removes test that verified ExternalServiceResource implements IResourceWithoutLifetime |
src/Aspire.Hosting/ResourceBuilderExtensions.cs |
Removes special case logic for waiting on ConnectionStringResource dependencies |
src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs |
Removes parameter.secret property from parameter resource properties |
src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs |
Changes parameter resource state from Active to Running |
src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs |
Removes connection string initialization logic and event subscription |
src/Aspire.Hosting/ExternalServiceResource.cs |
Removes IResourceWithoutLifetime interface implementation |
src/Aspire.Hosting/ConnectionStringResource.cs |
Removes IResourceWithoutLifetime interface implementation |
src/Aspire.Hosting/ConnectionStringBuilderExtensions.cs |
Adds initialization logic with dependency waiting and state management |
var tasks = new List<Task>(); | ||
foreach (var referencedResource in r.ConnectionStringExpression.ValueProviders.OfType<IResource>()) | ||
{ | ||
tasks.Add(evt.Notifications.WaitForResourceHealthyAsync(referencedResource.Name, ct)); |
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.
The code waits for resources to be healthy, but connection string resolution might only need the referenced resources to have their connection strings available, not necessarily be healthy. This could cause unnecessary delays or failures if a referenced resource is running but not yet healthy.
tasks.Add(evt.Notifications.WaitForResourceHealthyAsync(referencedResource.Name, ct)); | |
tasks.Add(evt.Notifications.WaitForResourceRunningAsync(referencedResource.Name, ct)); |
Copilot uses AI. Check for mistakes.
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.
I think this is a reasonable suggestion tbh. The resource does not need to be healthy to get the value of the connection string. That said, this affects consumers of the ConnectionStringAvailableEvent
, not users of ConnectionStringExpression
Would love feedback on:
|
@@ -216,7 +216,6 @@ internal static IResourceBuilder<T> AddParameter<T>(this IDistributedApplication | |||
{ | |||
ResourceType = KnownResourceTypes.Parameter, | |||
Properties = [ | |||
new("parameter.secret", resource.Secret.ToString()), |
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 is removed but I'm not seeing where it is injected elsewhere, are we just relying on parameter value now?
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.
I didn't need to remove it but it does nothing today 😄
Description
This comes after a long discussion on discord https://discord.com/channels/1361488941836140614/1402388529975263294 about the ability to WaitFor a parameter resource without using the value directly. Originally parameters we considered resources without a lifetime and we made it impossible to WaitFor them since their values could not change at runtime. With the new support to allow setting parameter values at runtime this has changed.
Also Fixes #10827
Checklist
doc-idea
templatebreaking-change
templatediagnostic
template