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.

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

[CMSSE-730]: Fix docs and warning for redis cache config #204

Merged

Conversation

angusjf
Copy link

@angusjf angusjf commented Jan 16, 2024

The config as I copied it from the readme didn't work - and there wasn't a warning for it just a failed pattern match.

I've created this mood board to explain the problem:

Screenshot 2024-01-16 at 14 43 52

Could well be the case that I've misread something but gave it a go to fix it. Possibly pattern matching on :'Elixir.EncryptedRedisTokenCache' isn't very nice because I guess fully qualified or aliased module names wouldn't match - I can't think of a nicer solution though.

@angusjf angusjf changed the title [CMSSE-730]: Add redis to boomerang to cache auth0 requests [CMSSE-730]: Fix docs and warning for redis cache config Jan 16, 2024
@angusjf angusjf marked this pull request as ready for review January 16, 2024 14:47
@angusjf angusjf requested a review from a team as a code owner January 16, 2024 14:47
Copy link
Contributor

@cottinisimone cottinisimone left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. Being that prima_auth0_ex, :redis, :enabled is deprecated there's no reason to let it live in the readme.

But maybe might be worth it to add another match after {true, nil} like:

{true, provider} ->
        Application.put_env(:prima_auth0_ex, :token_cache, provider)

?

Moreover, being that here you are, can you please fix the typo? It's written provieder XD.

If you think it's out of scope no worries, i'll open a card on our board with those changes

@angusjf
Copy link
Author

angusjf commented Jan 16, 2024

But maybe might be worth it to add another match after {true, nil} like:

{true, provider} ->
      Application.put_env(:prima_auth0_ex, :token_cache, provider)

?

@cottinisimone I don't get it, the provider is read from config so why would I put it back in the config?

@cottinisimone
Copy link
Contributor

But maybe might be worth it to add another match after {true, nil} like:

{true, provider} ->
      Application.put_env(:prima_auth0_ex, :token_cache, provider)

?

@cottinisimone I don't get it, the provider is read from config so why would I put it back in the config?

I'm just saying that some user might have an older version of auth0_ex so they might have enabled: true flag. If they set the provider it raise an exception. I'm just suggesting to gracefully handle this case so that it alerts you that the config is deprecated, but still it let you use the provider you put in the config

Co-authored-by: Simone Cottini <cottini.simone@gmail.com>
@angusjf angusjf requested a review from cottinisimone January 17, 2024 16:54
@angusjf angusjf merged commit ebbf4b7 into master Jan 22, 2024
3 checks passed
@angusjf angusjf deleted the CMSSE-730/task/add-redis-to-boomerang-to-cache-auth0-requests branch January 22, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants