-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add system env to all configs #49
Conversation
I just realized it's not this easy, I'll add tests and fix this to properly work |
lib/bamboo/adapters/smtp_adapter.ex
Outdated
@@ -286,6 +286,9 @@ defmodule Bamboo.SMTPAdapter do | |||
Enum.reduce(config, [], &to_gen_smtp_server_config/2) | |||
end | |||
|
|||
defp to_gen_smtp_server_config({:server, {:system, var}}, config) do | |||
[{:relay, System.get_env(var)} | config] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're at changing the code, what do you think of re-calling the function using the interpolated value? It could make it less error-prone if we don't duplicate the configuration keys.
Example (always easier 😸 )
defp to_gen_smtp_server_config({:server, {:system, var}}, config) do
to_gen_smtp_server_config({:server, System.get_env(var)}, config)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be easier to make it generic like this
defp to_gen_smtp_server_config({conf, {:system, var}}, config) do
to_gen_smtp_server_config({conf, System.get_env(var)}, config)
end
and then leave handling different types of configs in specific config for example tls requires atom and System.get_env returns string (this is actually why I realized it wasn't this easy)
I have now reworked settings system and added more validation to make sure values are always correct type. I've also added test for all configs variables to validate that system values are correctly transformed from string to proper format. |
Seems good to me. Could you just rebase this to one commit before we accept your PR? Thanks! |
Reworked smtp settings, added tests for system env
dd3745f
to
2c99515
Compare
It's now rebased to one commit |
This merge adds possibility to use system environment variables on all configs. Having only username and password makes it harder to change other settings when building released package with distillery.