Json endpoint #5
Conversation
This will prevent the need from retrieving the value for each and every call. This may not be overly important for env vars that are currently implemented. However, this could become a big cost saver for when you are retrieving your values from an external service. We also renamed the MyConfig.validate!/0 function to MyConfig.load!/0. You are now required to call `MyConfig.load!/0` before you try using your configuration as that is what will actually grab the values and store them into the cache.
This is meant to enable the user to retrieve values from a JSON endpoint. This also includes a way to make value names to how we want to reference them in our own systems. This means that if our json endpoint (or any other provider) gives us the key `foo-bar`, we can still use `foo_bar` in our own code. Please see `opt_7` in `json_endpoint_test.exs` for an example of how to do this. This should work for all providers, and not just for the JsonEndpoint. If the JsonEndpoint provider is unable to make the call to the specified endpoint, it will log a warning and pass nil values back to the caller and will still succeed assuming you have created default values for all params. This is mainly useful in development where you may not actually have another HTTP server running in order to retrieve values.
| [{Tesla.Middleware.BaseUrl, endpoint}, Tesla.Middleware.JSON] | ||
| |> Tesla.client() | ||
| |> Tesla.get("") |
There was a problem hiding this comment.
I'd probably use req (as an optional dep), and alternatively allow the client to supply the get function as an option. This still makes it possible (and reasonably easy) to use Tesla, but it doesn't force it, so those of us who prefer to use an HTTP client directly can easily do it.
| {display_name(k, spec), spec.default} | ||
| end) | ||
| |> Map.new() | ||
| |> Jason.encode!() |
There was a problem hiding this comment.
Could we use the stock Erlang's json (https://www.erlang.org/doc/apps/stdlib/json.html) instead? Note that in this case we'd only support OTP 27+.
There was a problem hiding this comment.
I would prefer not to as of right now. I would reconsider it once OTP 28 is released.
| Enum.map(params, fn {_k, _spec} -> | ||
| nil | ||
| end) |
There was a problem hiding this comment.
Is this actually what we want? Shouldn't we raise in this case?
I see you mention in the commit desc that
This is mainly useful in development where you may not actually
have another HTTP server running in order to retrieve values.
But, I don't think dev convenience justifies this default which can lead to weird prod bugs.
If the clients don't want to use a JSON server locally in dev, they could use a different source in dev. Or alternatively, start a mock server (for which we could support a convenience helper function, something like JsonEndpoint.test_server(port, params).
There was a problem hiding this comment.
if the clients don't want to use a JSON server locally in dev, they could use a different source in dev.
This is very true. There is no reason we could not have someone do something like
defmodule MyConfig do
use Provider,
source: Application.compile_env(:my_app, :my_config_source)
endin order to do something slightly different between dev and prod-like deployments.
| @spec fetch_all(source, params) :: {:ok, data} | {:error, [String.t()]} | ||
| def fetch_all(source, params) do | ||
| @spec fetch_all(source, params, Keyword.t()) :: {:ok, data} | {:error, [String.t()]} | ||
| def fetch_all(source, params, opts) do |
There was a problem hiding this comment.
These opts are passed to the source, right? It would be good to document that somewhere.
| end) | ||
| |> Enum.flat_map(fn {key, errors} -> | ||
| Enum.map(errors, &"#{source.display_name(key)} #{&1}") | ||
| Enum.map(errors, &"#{source.display_name(key, params[key])} #{&1}") |
There was a problem hiding this comment.
Don't quite understand why is this change needed.
There was a problem hiding this comment.
Unfortunately I can't say I remember the details behind it :(
There was a problem hiding this comment.
That's on me, should have reviewed this much sooner 😅
|
I think we can actually close this one. I don't work on the project that needs this anymore, and if the need comes up again, we can just recreate it. I think focusing on #4 makes the most sense for now. |
2 participants
This builds on top of #4.