Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#2054 #2059 Manage EnableContentHashing setting by global CacheOptions #2058

Merged
merged 20 commits into from
May 13, 2024
Merged

#2054 #2059 Manage EnableContentHashing setting by global CacheOptions #2058

merged 20 commits into from
May 13, 2024

Conversation

thiagoloureiro
Copy link
Contributor

@thiagoloureiro thiagoloureiro commented Apr 30, 2024 •
edited by raman-m

Fixes #2059

Proposed Changes

This PR is prepared to fix the EnableContentHashing not being considered on ocelot.json.

FileCacheOptions didn't have the option to fetch from the config and also the RoutesCreator didn't call the overload with the settings.

@raman-m
Copy link
Member

raman-m commented May 1, 2024

Thank you, Thiago!
What issue is it related to?

@raman-m raman-m added proposal Proposal for a new functionality in Ocelot Summer'24 Summer 2024 release labels May 1, 2024
@raman-m raman-m added this to the May-June'24 milestone May 1, 2024
ggnaegi
ggnaegi reviewed May 1, 2024
View reviewed changes
Copy link
Member

@ggnaegi ggnaegi left a comment

Choose a reason for hiding this comment

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

Hello thanks, it's fine, but I would like to check the CacheOptions creation. It looks a bit like a hack...

src/Ocelot/Configuration/Creator/RoutesCreator.cs Outdated Show resolved Hide resolved
@raman-m raman-m changed the title EnableContentHashing not being considered from appsettings EnableContentHashing not being considered from appsettings.json May 2, 2024
@ggnaegi
Adding CacheOptionsCreator, Injected IRegionCreator as Singleton. Sho…
16abde6 
…uld still add some acceptance tests that are definitely missing!
@ggnaegi
Copy link
Member

ggnaegi commented May 9, 2024

It is related to the following issue: #2059

@ggnaegi
Adding caching global configuration since we messed up, ignoring an i…
c800b4f 
…mportant breaking change with EnableContentHashing set to false by default
@ggnaegi
Copy link
Member

ggnaegi commented May 10, 2024 •
edited

@raman-m @thiagoloureiro I have added a global configuration for the cache options. Since we introduced a breaking change by adding the property EnableContentHashing and setting it to false by default, I think the users shouldn't bear the burden. That's why I thought adding the global configuration point EnableContentHashing would help most of them. The documentation will be updated too.

@ggnaegi ggnaegi added accepted Bug or feature would be accepted as a PR or is being worked on Caching Ocelot feature: Caching critical Critical priority, higher than Highest. It requires hotfix ASAP Spring'24 Spring 2024 release and removed proposal Proposal for a new functionality in Ocelot Summer'24 Summer 2024 release labels May 10, 2024
@ggnaegi ggnaegi self-assigned this May 10, 2024
@ggnaegi
Copy link
Member

ggnaegi commented May 10, 2024

@raman-m It's ready for review. I'm still using Bddfy, it was straightforward. And there is a silly problem the application is expecting TtlSeconds to be explicitly set for each route, since TtlSeconds > 0 -> IsCached.

@raman-m raman-m linked an issue May 11, 2024 that may be closed by this pull request
FileCacheOptions not working after the header was introduced in FileCache settings in version 23.0.0 #2059
Closed
@raman-m raman-m modified the milestones: May-June'24, March-April'24 May 11, 2024
@raman-m raman-m changed the title EnableContentHashing not being considered from appsettings.json #2054 #2059 Manage EnableContentHashing setting by global CacheOptions May 11, 2024
@raman-m raman-m removed the critical Critical priority, higher than Highest. It requires hotfix ASAP label May 11, 2024
@ggnaegi ggnaegi requested a review from raman-m May 12, 2024 20:33
@ggnaegi
Copy link
Member

ggnaegi commented May 12, 2024 •
edited

@raman-m I have added some unit tests for the CacheOptionsCreator, and implemented most of your suggestions

raman-m
raman-m approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Technically we could merge, but I'd like to propose more...
Wait for my next commits plz!

src/Ocelot/Configuration/Creator/CacheOptionsCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/File/FileGlobalConfiguration.cs Show resolved Hide resolved
test/Ocelot.UnitTests/Cache/RegionCreatorTests.cs Outdated Show resolved Hide resolved
ggnaegi and others added 2 commits May 13, 2024 16:26
@ggnaegi
Copy link
Member

ggnaegi commented May 13, 2024

@raman-m it's sound for me, should I squash and merge?

@raman-m
Copy link
Member

raman-m commented May 13, 2024

@ggnaegi Please don't merge yet! Hold on for a 10-20 minutes... I want to present a commit related to the property name.

@raman-m
Copy link
Member

raman-m commented May 13, 2024

@ggnaegi The commit found at 3290aa6 reflects my earlier suggestion regarding the proposed renaming. I'm confident that this commit does not introduce any breaking changes. Please share your thoughts.

@raman-m
Copy link
Member

raman-m commented May 13, 2024

@ggnaegi commented:

it's sound for me, should I squash and merge?

Yes, please do, provided you have no objections to my last commit 3290aa6.

@raman-m
Copy link
Member

raman-m commented May 13, 2024 •
edited

Ready for delivery ✅

  • Code review ✔️✔️✔️
  • Team approvals ✔️✔️✔️
  • Unit testing ✔️ → CacheOptionsCreatorTests → link
  • Acceptance testing ✔️ → CachingTests → 2 tests
  • Updated feature docs ✔️ → caching.rst → link

@ggnaegi
Copy link
Member

ggnaegi commented May 13, 2024

@raman-m I'm not sure about FileCacheOptions being renamed to CacheOptions. I would prefer keeping FileCacheOptions for now. For me it was all sound and we are going now a step too far. I would prefer having those changes in another PR.

@raman-m
Copy link
Member

raman-m commented May 13, 2024

@ggnaegi Understood! Reverting...

@raman-m
Copy link
Member

raman-m commented May 13, 2024 •
edited

@ggnaegi The recent changes have been reverted❕ The proof -> link
I think we're ready to merge now. 😃
You could press the magic button 😜

@ggnaegi
Copy link
Member

ggnaegi commented May 13, 2024

@raman-m Ok, I got it, I just realized I named the Global FileCacheOptions CacheOptions... Well... Are you ok with that, and we announce the community that in a next version we will rename it?

@ggnaegi ggnaegi merged commit 6e9a975 into ThreeMammals:develop May 13, 2024
1 check passed
@raman-m raman-m removed the accepted Bug or feature would be accepted as a PR or is being worked on label May 13, 2024
@raman-m
Copy link
Member

raman-m commented May 13, 2024 •
edited

TODO

  • Rename FileCacheOptions to CacheOptions post-release

@thiagoloureiro
Copy link
Contributor Author

thanks for adding all the comments and commits guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raman-m raman-m raman-m approved these changes

@RaynaldM RaynaldM RaynaldM approved these changes

@ggnaegi ggnaegi ggnaegi approved these changes

Assignees

@thiagoloureiro thiagoloureiro

@ggnaegi ggnaegi

Labels
bug Identified as a potential bug Caching Ocelot feature: Caching highest Highest priority hotfix Gitflow: Hotfix issue, PR related to hotfix branch Spring'24 Spring 2024 release
Projects
None yet
Milestone
Spring'24
Development

Successfully merging this pull request may close these issues.

FileCacheOptions not working after the header was introduced in FileCache settings in version 23.0.0
4 participants
@thiagoloureiro @raman-m @ggnaegi @RaynaldM