-
Notifications
You must be signed in to change notification settings - Fork 850
Fix event order and callback calling during container creation #15503
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?
Changes from all commits
b9f75f3
276a818
7f47b28
c3939f0
c3fa92a
5088bed
312e6ba
478416b
a2a732f
27771ad
77f43b7
9e2a0a5
aebce27
3532eed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| namespace Aspire.Hosting.ApplicationModel; | ||
|
|
||
| /// <summary> | ||
| /// Represents a resource annotation whose callback should be evaluated at most once, | ||
| /// with the result cached for subsequent retrievals. | ||
| /// </summary> | ||
| /// <typeparam name="TContext">The type of the context passed to the callback.</typeparam> | ||
| /// <typeparam name="TResult">The type of the result produced by the callback.</typeparam> | ||
| internal interface ICallbackResourceAnnotation<TContext, TResult> | ||
| { | ||
| /// <summary> | ||
| /// Evaluates the callback if it has not been evaluated yet, caching the result. | ||
| /// Subsequent calls return the cached result regardless of the context passed. | ||
| /// </summary> | ||
| /// <param name="context">The context for the callback evaluation. Only used on the first call.</param> | ||
| /// <returns>The cached result of the callback evaluation.</returns> | ||
| Task<TResult> EvaluateOnceAsync(TContext context); | ||
|
|
||
| /// <summary> | ||
| /// Clears the cached result so that the next call to <see cref="EvaluateOnceAsync"/> will re-execute the callback. | ||
| ///</summary> | ||
| /// <remarks> | ||
| /// Use <see cref="ForgetCachedResult"/> when a resource decorated with this callback annotation is restarted. | ||
| /// </remarks> | ||
| void ForgetCachedResult(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,19 +6,26 @@ namespace Aspire.Hosting.ApplicationModel; | |
| /// <summary> | ||
| /// Specifies how resource dependencies are discovered. | ||
| /// </summary> | ||
| [Flags] | ||
| public enum ResourceDependencyDiscoveryMode | ||
| { | ||
| /// <summary> | ||
| /// Discover the full transitive closure of all dependencies. | ||
| /// This includes direct dependencies and all dependencies of those dependencies, recursively. | ||
| /// </summary> | ||
| Recursive, | ||
| Recursive = 1, | ||
|
|
||
| /// <summary> | ||
| /// Discover only direct dependencies. | ||
| /// This includes dependencies from annotations (parent, wait, connection string redirect) | ||
| /// and from environment variables and command-line arguments, but does not recurse | ||
| /// into the dependencies of those dependencies. | ||
| /// </summary> | ||
| DirectOnly | ||
| DirectOnly = 2, | ||
|
|
||
| /// <summary> | ||
| /// When set, unresolved values from annotation callbacks will be cached and reused | ||
| /// on subsequent evaluations of the same annotation, rather than re-evaluating the callback each time. | ||
| /// </summary> | ||
| CacheAnnotationCallbackResults = 4 | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With Also,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should CacheAnnotationCallbackResults be public? Will people other than us ever use it? If not, you could remove it from the enum (and make it no longer flags) and have an extra bool on internal GetDependenciesAsync to control the behavior.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you are right, I was too clever here. I decided to keep
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still not a fan of making the enum a flags enum. DirectOnly and Recursive are opposites of each other. What happens when both are set? Should the extra flag be a new parameter instead of adding to the mode enum and making it a flags enum?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JamesNK You get an There are many examples of In our case I think the benefits of having a single
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be fair, a new major mode can be added equally well with split parameters
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be fair, another major mode can be added equally well with two separate parameters.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why it's preferable to squash options into enum when we don't have to. Personally, I think the best option would be to add a new overload that just has an On the options you can set the mode, and a bool flag CacheAnnotationCallbackResults to indicate whether results are cached or not. Any future arguments for GetResourceDependencies (this seems like an API that would get more) could then be added to the options without adding new overloads or breaking APIs. And the options class can be passed down through all the other methods. |
||
Uh oh!
There was an error while loading. Please reload this page.