Add WithCache option to for NewKeyRetriever #166
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mem
requested review from
cinaglia and
colin-stuart
and removed request for
a team
mem
commented
Apr 2, 2025
mem
force-pushed
the
mem/add-with-cache-opt
branch
2 times, most recently
from
3d2ffe8 to
8fc6d99
Compare
colin-stuart
approved these changes
May 19, 2025
mem
force-pushed
the
mem/add-with-cache-opt
branch
from
8fc6d99 to
cda1623
Compare
It might be worth offering the option to set a custom cache, possibly with different policies or with a different caching store. This is almost supported, as cache.Cache is an interface, so it's possible to reimplement it. This could be done using a DefaultKeyRetrieverOption function to pass to NewKeyRetriever that can replace the cache field `c`. The problem is that the field is not exported, so it's not possible to implement the option outside of the package, and the type does not offer a function to set the private field (such a function would make the option unnecessary). The same goes for the TokenExchangeClient. Add those options in this change. It needs to take into consideration the fact that cache.NewLocalCache ends up starting a goroutine to do clean up tasks. While the goroutine should stop because the cache has been replaced, it's better to not start it in the first place. Signed-off-by: Marcelo E. Magallon <marcelo.magallon@grafana.com>
mem
force-pushed
the
mem/add-with-cache-opt
branch
from
cda1623 to
5b452ab
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It might be worth offering the option to set a custom cache, possibly with different policies or with a different caching store.
This is almost supported, as cache.Cache is an interface, so it's possible to reimplement it. This is done using a
DefaultKeyRetrieverOption function to pass to NewKeyRetriever that can replace the cache field
c. The problem is that the field is not exported, so it's not possible to implement the option outside of the package.Add that option here. It needs to take into consideration the fact that cache.NewLocalCache ends up starting a goroutine to do clean up tasks. While the goroutine should stop because the cache has been replaced, it's better to not start it in the first place.