Allow dynamically starting strategies + expand GenServer-ness of the DefaultStrategyTemplate #39
Conversation
lovebes
commented
Oct 1, 2022
lovebes
commented
- Expaneded the GenServer-ness of the DefaultStrategyTemplate
- Changed some of the behavior callback arity
- Created DynamicSupervisor and Registry to be used
- Added tests to show examples of doing dynamically generating strategies.
… setting up only one module for one strategy, and not useable for dynamic spinning up of strategies
Co-authored-by: Victor Oliveira Nascimento <376386+victorolinasc@users.noreply.github.com>
…with errors when running during start_link
…n by atom and string
…via GenServer.call, and trap exit to ensure ets removal
… called via GenServer.call, and trap exit to ensure ets removal" This reverts commit 04fae8a.
…via GenServer.call, and trap exit to ensure ets removal
lovebes
changed the title
|
I am reviewing this at the velocity of a turtle... but I am looking into it. Sorry for the long delays here! |
|
@lovebes I was reviewing and testing this but I got the conclusion we need some bigger changes here... For instance, issue #37 . We need to be more compliant with the specification about possible kidless and duplicated kids in a set. So, I am gathering feedback to a breaking change which would allow us to change the strategies here. Maybe we should rename the current strategy to something like Other than that, an issue with synchronous initialization was caught that simply showed that we are not properly initializing the ETS here. This is some ancient code that I want to refactor properly too and not maintain it like it is now forever. So, I've pushed a change with the CI exactly like it is currently in Joken and some logging improvements. This broke your PR here but I wanted to tell you that your help will be much appreciated when we start tackling 2.0. Your code here is an eye opener for these different strategies. So, first of all, thanks a lot :) Secondly, let's discuss better what other issues and uses we might want with a breaking change. Wdyt? |
|
@victorolinasc I like all of your decisions, and yeah having read #37 I see what you mean. I am all for a 2.0 for this very important library. I don't think there's anything like it in the Elixir ecosphere that does Joken and JWKS simply and would like for it to have more support. I like how you want to keep the current strategy as-is, as that was my original intention but wanted to keep the footprint small and not introduce more files ;) So yeah please ping me when you see I might be of use for 2.0, etc - thank you for this library! Feel free to close this PR, leave it open, whichever you prefer! |
|
|
||
| should_start -> | ||
| {:ok, _} = start_fetch_signers(opts[:jwks_url], opts) | ||
| Task.start_link(__MODULE__, :poll, [opts]) | ||
| fetch_signers(opts[:jwks_url], opts) |
There was a problem hiding this comment.
👋 we're currently trying to debug in our codebase related to first_fetch_sync and saw this change on master: Shouldn't the !first_fetch_sync case not include this line? Seems odd that the first two branches of this cond are the same.
There was a problem hiding this comment.
https://github.com/joken-elixir/joken_jwks/pull/39/files#diff-c971e4f3329485ad6a91eb81a345c2144a3b3eb6a035bad9f84395c1fcde24a0L267 in the original code also runs fetch_signers - but done in a Task.async.
It's been a while since I created the PR and I might have forgotten the nuances, but it seems I saw what was happening and decided that because of dynamically generated strategies (hence not really a need for doing Task.async), and to maintain parity with original code, start_fetch_signers was removed and the code inside it was used, hence fetch_signers.
@victorolinasc correct me if I'm wrong - I might have missed a layer of understanding here.
