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

Surface retriever startup error #165

Merged

Conversation

danschultzer
Copy link
Contributor

@danschultzer danschultzer commented Feb 29, 2024

If you don't set the :default_cldr_backend for :ex_money (I had set for :ex_cldr), the retriever just silently fails start up, and subsequent exchange rates requests fails. Took me a sec to find out it was because the retriever just silently fails.

This change will surfaces any startup error by naively raising the reason, but I think there may be a better way to handle it or format the error. Not really a fan of this solution. I haven't added a test case for it as I couldn't find any tests for the exchange rates supervisor), and again, there's probably a better way to handle the upstart error inside the supervision tree.

The simplest way to test it is to change the test config to:

config :ex_money,
  auto_start_exchange_rates_service: true,
  exchange_rates_retrieve_every: :timer.minutes(1),
  api_module: Money.ExchangeRates.OpenExchangesRates,
  # exchange_rates_retrieve_every: :never,
  # open_exchange_rates_app_id: {:system, "OPEN_EXCHANGE_RATES_APP_ID"},
  # api_module: Money.ExchangeRates.Api.Test,
  log_failure: nil,
  log_info: nil
  # default_cldr_backend: Test.Cldr

The other thing is to figure out why Money wouldn't fall back to the :ex_cldr :default_cldr_backend config for the supervisor.

@kipcole9
Copy link
Owner

Another great catch.

Having a default CLDR backend is pretty much a requirement for this lib so I think I can check for that in Application.start/0 and raise there if its not configured, or can't be resolved from Cldr.default_backend/0.

I'll work on this later this morning my time. Thanks again.

@kipcole9 kipcole9 merged commit 344ae6d into kipcole9:main Apr 20, 2024
1 check passed
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.

None yet

2 participants