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

Allow connection details to be overridden #22

Closed
wants to merge 2 commits into from

Conversation

dparnell
Copy link

@dparnell dparnell commented Jul 2, 2016

Hi,

for my application I need to be able to change the host and port at run time based on config passed into the app from outside. I have added a little bit of code to allow the config to be altered at run time.

I've added a test for this and all the other tests that were passing before my change are still passing.

Daniel

@dparnell
Copy link
Author

dparnell commented Jul 2, 2016

Just realised this isn't going to work as the agent is created at macro expansion time, not run time 😢 I will have to work on it some more

@dparnell
Copy link
Author

dparnell commented Jul 2, 2016

OK, that works now as intended :)

@mneudert
Copy link
Owner

mneudert commented Jul 4, 2016

Yep, the code works as intended and tells me there is an issue with the overall way the configuration is handled 👍

The agent is a possible solution to runtime configuration changes but we might get a supervision problem with it. Currently only the worker pool is supervised and the new agent might die and never get restarted. So the real issue you have might only get moved around but not really solved.

For starters there is a split between compile time and runtime configuration. Until any issue arises with the logging modules needing to be swapped during runtime I would keep those as the only compile time configuration. Everything else would fall into the runtime category being completely changeable to suit all needs.

Accessing those runtime values could be done using Application.get_env/2 (probably the easiest way) or a custom agent hooked up into supervision along the pool (drops that @on_load you needed, feels "hacky", doesn't it?). The agent would always have the problem of perhaps loosing your configuration during a process restart. An :ets table with a :heir configuration solves that but seems more complex than just using what is already available.

Whatever is used at the end would also open the way to have { :system, "MY_ENV_VAR" } to fetch configuration directly from the system environment. And that might be the even better change at the end.

Or is there any special use case I can't think of where the current agent seems like a more suitable way?

@dparnell
Copy link
Author

dparnell commented Jul 4, 2016

Yeah, I didn't like the @on_load. It seems really nasty to me. I'm fairly new to Elixir and didn't want to go making too many changes. As for why I chose the agent stuff the reading I did seemed to say this was the "Elixir" way of storing state.

@mneudert
Copy link
Owner

mneudert commented Jul 5, 2016

I just pushed an update to master supporting runtime configuration changes using Application.put_env/3. This would be the way to change values now:

new_config = Keyword.put(ConnectionModule.config(), :host, "changed.host")
:ok        = Application.put_env(:your_otp_app, ConnectionModule, new_config)

Essentially what you already did but using the application environment that is used to access the configuration in the first place (and was before the logger introduction). Agents are great for storing state as you said but this is more an application configuration thing here. This way we don't have to think about where configuration lives and also prevent any possible supervision workarounds.

Could you please verify this works for you?

@dparnell
Copy link
Author

dparnell commented Jul 6, 2016

This looks exactly like what I need 👍
Thanks for your help 😄

@mneudert
Copy link
Owner

mneudert commented Jul 6, 2016

Great to hear, next steps will be some internal improvements and access to the system environment. Thanks for bringing this issue up and helping to resolve it!

@mneudert mneudert closed this Jul 6, 2016
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.

2 participants