Skip to content

Simplify redis cache setup #49722

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

Closed
wants to merge 9 commits into from
Closed

Conversation

tebeco
Copy link
Contributor

@tebeco tebeco commented Jul 29, 2023

Add flexibility to AddStackExchangeRedisCache by removing setupAction

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Adds an overload to AddStackExchangeRedisCache without previously mandatory setupAction

Description

Add flexibility to AddStackExchangeRedisCache by removing setupAction

Fixes #49721

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Jul 29, 2023
@ghost
Copy link

ghost commented Jul 29, 2023

Thanks for your PR, @tebeco. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@tebeco tebeco force-pushed the simplify-redis-cache-setup branch from 2581e3e to ab452b1 Compare July 29, 2023 11:33
@tebeco tebeco force-pushed the simplify-redis-cache-setup branch 2 times, most recently from f307d0b to c55dc79 Compare July 29, 2023 12:02
@tebeco tebeco requested a review from a team as a code owner July 29, 2023 12:27
@mgravell
Copy link
Member

Interestingly, the fact that there may be duplicated code between distributed cache and output cache came up separately, and may be a parallel motivation for this as a change. I need to check on API consistency, though.

@tebeco tebeco force-pushed the simplify-redis-cache-setup branch from a05a20b to c55dc79 Compare July 29, 2023 17:13
@tebeco
Copy link
Contributor Author

tebeco commented Jul 29, 2023

Am I supposed ... or not to change the PublicApi files ...
because I'm seeing contradictory failures such as:
image

But when I add it, it's telling me it shouldn't be updated and repeating the docs which says:
image

Is that supposed to be an error and a repo maintainer adds a commit to the PR ?

@tebeco
Copy link
Contributor Author

tebeco commented Jul 29, 2023

Interestingly, the fact that there may be duplicated code between distributed cache and output cache came up separately, and may be a parallel motivation for this as a change. I need to check on API consistency, though.

Fine for me, hopping that it can land in net8.0 since i'm probably short in time with the release / RC ;)

@BrennanConroy
Copy link
Member

https://github.com/dotnet/aspnetcore/blob/main/docs/APIBaselines.md#new-apis

A new entry needs to be added to the PublicAPI.Unshipped.txt file for a new API. For example:

@tebeco tebeco force-pushed the simplify-redis-cache-setup branch from a9b4ae2 to 81cc5d9 Compare July 29, 2023 17:26
@tebeco
Copy link
Contributor Author

tebeco commented Jul 29, 2023

https://github.com/dotnet/aspnetcore/blob/main/docs/APIBaselines.md#new-apis

A new entry needs to be added to the PublicAPI.Unshipped.txt file for a new API. For example:

Ah ! thanks
I think I got it now, I though Shipped and Unshipped had other meaning.
So basically the Unshipped file is sort of the "next to be Shipped" Apis that will be added to the already Shipped from previous release ?

Hope I didn't mess the files/TFM since OutputCache is declared with #if NET7_0_OR_GREATER:

  • AddStackExchangeRedisCache in all TFM
  • AddStackExchangeRedisOutputCache only in the net8.0

@mgravell
Copy link
Member

To set expectations: we'll have a look at this and discuss things internally but: we're rapidly approaching lockdown for net8, especially since this involves API changes.

@adityamandaleeka
Copy link
Member

@mgravell I've assigned the issue (#49721) to you to drive figuring out the design.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Aug 9, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
@mgravell
Copy link
Member

mgravell commented Nov 8, 2023

@tebeco as I'm sure you know, we didn't manage to get to this in net8, sorry - not least because of some last minute changes to the assembly layout. Now that we're coming up from air a bit, we can try to catch up with this; overall, it looks reasonable, but we'll need to reflect those changes, specifically: reflect the move of the bits to here. I've had a look at this locally, and it looks like it would be quicker to redo from "main" than to fix those conflicts, but I don't want to jump in an remove credit on the PR. Are you eager enough to update to reflect that? Alternatively, I can take it over - copying over the proposed changes. Thoughts?

@tebeco tebeco force-pushed the simplify-redis-cache-setup branch from 81cc5d9 to d401918 Compare November 25, 2023 09:47
@tebeco
Copy link
Contributor Author

tebeco commented Nov 25, 2023

@tebeco as I'm sure you know, we didn't manage to get to this in net8, sorry - not least because of some last minute changes to the assembly layout. Now that we're coming up from air a bit, we can try to catch up with this; overall, it looks reasonable, but we'll need to reflect those changes, specifically: reflect the move of the bits to here. I've had a look at this locally, and it looks like it would be quicker to redo from "main" than to fix those conflicts, but I don't want to jump in an remove credit on the PR. Are you eager enough to update to reflect that? Alternatively, I can take it over - copying over the proposed changes. Thoughts?

I just realized i did not read through all your message, I just rebased the branch on top of the change I've seen.
If you think you need additional change, you're absolutly free to add more commit onto that branch / PR and go on

image

this should be enabled as well

@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment Feb 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@wtgodbe wtgodbe removed the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 20, 2024
@tebeco
Copy link
Contributor Author

tebeco commented Feb 20, 2024

nop this is still active from the owner/op point of view

@tebeco
Copy link
Contributor Author

tebeco commented Feb 20, 2024

/azp run

Copy link

Commenter does not have sufficient privileges for PR 49722 in repo dotnet/aspnetcore

@adityamandaleeka
Copy link
Member

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 26, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@adityamandaleeka
Copy link
Member

@mgravell Just making sure this is this still on your radar

@adityamandaleeka adityamandaleeka added pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun and removed pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun labels Feb 26, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 5, 2024
@tebeco
Copy link
Contributor Author

tebeco commented Mar 5, 2024

🫥

@tebeco
Copy link
Contributor Author

tebeco commented Jun 16, 2024

🦤

@danmoseley danmoseley requested a review from Copilot February 14, 2025 04:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 8 changed files in this pull request and generated no comments.

Files not reviewed (4)
  • src/Caching/StackExchangeRedis/src/PublicAPI/net462/PublicAPI.Unshipped.txt: Language not supported
  • src/Caching/StackExchangeRedis/src/PublicAPI/net9.0/PublicAPI.Unshipped.txt: Language not supported
  • src/Caching/StackExchangeRedis/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt: Language not supported
  • src/Middleware/Microsoft.AspNetCore.OutputCaching.StackExchangeRedis/src/PublicAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (1)

src/Caching/StackExchangeRedis/src/StackExchangeRedisCacheServiceCollectionExtensions.cs:21

  • The tag should be updated to match the existing documentation style. It should be 'The so that additional calls can be chained.'.
<returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>

@tebeco tebeco closed this by deleting the head repository Mar 31, 2025
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview4 milestone Mar 31, 2025
@mgravell
Copy link
Member

echoing comment here: #49721 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add flexibility to AddStackExchangeRedisCache by removing setupAction
7 participants