Skip to content

Comments

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

Merged
ggnaegi merged 20 commits intoThreeMammals:developfrom
thiagoloureiro:fix/EnableContentHashing_missing
May 13, 2024
Merged

#2054 #2059 Manage EnableContentHashing setting by global CacheOptions#2058
ggnaegi merged 20 commits intoThreeMammals:developfrom
thiagoloureiro:fix/EnableContentHashing_missing

Conversation

@thiagoloureiro
Copy link
Contributor

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

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 Oct'24 October 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
Collaborator

@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...

@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
Collaborator

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
Collaborator

ggnaegi commented May 10, 2024 •
edited
Loading

@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 Oct'24 October 2024 release labels May 10, 2024
@ggnaegi ggnaegi self-assigned this May 10, 2024
@ggnaegi
Copy link
Collaborator

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
Collaborator

ggnaegi commented May 12, 2024 •
edited
Loading

@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!

ggnaegi and others added 2 commits May 13, 2024 16:26
@ggnaegi
Copy link
Collaborator

ggnaegi commented May 13, 2024

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

RaynaldM
RaynaldM approved these changes May 13, 2024
View reviewed changes
@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
Loading

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
Collaborator

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
Loading

@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
Collaborator

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
@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
Loading

Tasks

@thiagoloureiro
Copy link
Contributor Author

thiagoloureiro commented May 15, 2024

thanks for adding all the comments and commits guys!

This was referenced Jun 5, 2024
Closed
Merged
This was referenced Dec 13, 2024
Merged
Closed
@raman-m raman-m mentioned this pull request Aug 12, 2025
Closed
@raman-m raman-m added the Configuration Ocelot feature: Configuration label Oct 27, 2025
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

+2 more reviewers

@RaynaldM RaynaldM RaynaldM approved these changes

@ggnaegi ggnaegi ggnaegi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@thiagoloureiro thiagoloureiro

@ggnaegi ggnaegi

Labels

bug Identified as a potential bug Caching Ocelot feature: Caching Configuration Ocelot feature: Configuration 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