Skip to content

Conversation

@lucasavila00
Copy link
Contributor

Would fix #18

Copy link
Owner

@Nickforall Nickforall left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR :)

I'm not sure if this is the right approach because I'd always want my app to to fail whenever there are no public keys stored on production environments. If one prod instance fails to fetch for whatever reason it's essentially useless because it can't authenticate anyone, however with this change it will not report any error that indicates it's useless.

In large scale applications this might be really problematic because one of my instances may silently be unable to do anything. Rendering a percentage of the requests useless..

I want to propose this approach:

Add an application env along the lines of

config :ex_firebase_auth, :key_store_fail_strategy, :stop

Which can take the values :stop, :warn and :silent. Default will be stop

:stop will be the current behavior. I'd change the error message to indicate there's a way to prevent the application from crashing by adding the above code snippet to your (test) config.
:warn will print warnings with Logger and refresh
:silent will only call refresh and not do anything

Let me know what you think and if you want to pick this up, i'm fine with picking this up myself as well. Doesn't really need tests imo so should be a quick change.

@lucasavila00
Copy link
Contributor Author

Sorry for the delay, these last days have been busy.

I think I did it correctly, and I don't know how to test that too..

Should it have something written in the documentation?

I was going to write it in the Readme but then I saw that the documentation has a message saying it's auto-generated...

Please let me know how to handle that.

Thanks :)

Copy link
Owner

@Nickforall Nickforall left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution, this is super helpful :)

@Nickforall
Copy link
Owner

Should it have something written in the documentation?

I wanna re-do the readme anyways based on feedback from other contributors. Will do that before releasing 0.4.1

@Nickforall Nickforall merged commit a80b40c into Nickforall:main May 7, 2021
@lucasavila00
Copy link
Contributor Author

🚀 🎊

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.

[Feature Request] Don't prevent applications from starting up if credentials cannot be fetched

2 participants