[Container] Add OutputCache configuration from file #2883
Conversation
src/ReverseProxy/Configuration/Middlewares/OutputCacheConfig.cs
Outdated
Show resolved
Hide resolved
src/ReverseProxy/Configuration/Middlewares/OutputCacheConfig.cs
Outdated
Show resolved
Hide resolved
src/ReverseProxy/Configuration/Middlewares/OutputCacheConfig.cs
Outdated
Show resolved
Hide resolved
src/ReverseProxy/Configuration/Middlewares/OutputCacheConfig.cs
Outdated
Show resolved
Hide resolved
src/ReverseProxy/Configuration/Middlewares/OutputCacheConfig.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Should this be in the container assembly instead?
Long-term this is something that should likely be in ASP.NET instead of YARP.
There was a problem hiding this comment.
That's something I asked myself too, but I want to be able to reference these types in Aspire later...
I coud just duplicate the code for now in Aspire, that's a possibility. It would avoid the need of publishing a nuget package for that.
There was a problem hiding this comment.
I suppose it's a question of whether we see this being valuable to YARP users outside of Aspire.
I don't have a problem with it shipping from YARP initially, we could always take the breaking change eventually if ASP.NET starts to include it.
Good point re: package update, if we're treating this as more experimental, then I think it'd be more flexible for us if we copy-paste it in Aspire instead, and keep it just in the container here for now.
| /// <summary> | ||
| /// Collections of extensions to configure OutputCache | ||
| /// </summary> | ||
| public static class OutputCacheConfigExtensions |
There was a problem hiding this comment.
Nit: I think we've mostly stuck with one type per file so far
There was a problem hiding this comment.
Meh, we have modern IDE that supports multiple types per file. I think when the types are small and related, I find it easier to keep it together, if the file isn't big of course.
There was a problem hiding this comment.
This type specifically should likely be in namespace Microsoft.Extensions.DependencyInjection (doesn't matter if it's not public)
|
Hope you don't mind me asking - looking at this PR, it seems like dynamic operations on |
| /// <summary> | ||
| /// Configuration for <see cref="OutputCachePolicyBuilder"/> | ||
| /// </summary> | ||
| public sealed record NamedCacheConfig |
There was a problem hiding this comment.
We should either move this to a more nested namespace, or name this NamedOutputCacheConfig, just cache config is too generic for Yarp.ReverseProxy.Configuration IMO.
There was a problem hiding this comment.
I suppose it's a question of whether we see this being valuable to YARP users outside of Aspire.
I don't have a problem with it shipping from YARP initially, we could always take the breaking change eventually if ASP.NET starts to include it.
Good point re: package update, if we're treating this as more experimental, then I think it'd be more flexible for us if we copy-paste it in Aspire instead, and keep it just in the container here for now.
| /// <summary> | ||
| /// Collections of extensions to configure OutputCache | ||
| /// </summary> | ||
| public static class OutputCacheConfigExtensions |
There was a problem hiding this comment.
This type specifically should likely be in namespace Microsoft.Extensions.DependencyInjection (doesn't matter if it's not public)
| Assert.NotNull(test1); | ||
| Assert.NotNull(test2); | ||
| Assert.NotNull(test3); |
There was a problem hiding this comment.
I take it there's no good way to assert that the relevant properties were applied correctly?
When using the YARP container, configuring middleware via dependency injection (DI) is not an option. This limitation makes it difficult to customize middleware behavior without modifying and rebuilding the container image.
In this PR, I added the possibility to configure output caching in the container configuration file.