python-consul
python-consul copied to clipboard
cablehead
RFC: Implement support for connecting to consul via unix socket
Basically I need python-consul to work with this:
# env | grep CONSUL_HTTP_ADDR
CONSUL_HTTP_ADDR=unix:///var/run/consul/http.sock
Just like the consul cli does.
Unfortunately I had to change a lot of code to make this work :-( Basically the uri (unix:///var/run/consul/http.sock) has to be passed to all the different implementations like std, aio, tornado as they all handle this type of connection differently. I implemented the feature for all the backends I'm familiar with std and aio but not tornado and twisted.
I also removed the default environment variables support in base.py for 2 reasons:
- The existing implementation interfered with the feature I wanted to implement.
- I strongly believe a environment variable should never override an explicitly defined argument.
To fully support configuring a client through environment variables I added a explicit helper function similar to how the docker-py project implements it.
c = consul.Consul.from_env()
It looks like those test failures are just flake8 errors with python3.
This looks great. Do you have time to research adding tornado + twisted support? It'd be good to keep things consistent. If not we could try and recruit someone to help out.
If you can fix the test suite, and add Tornado + Twisted, I'd love to merge and push this to PyPI.
FWIW this pull request also fixes #192. IMHO more elegantly then the impl in #193.
@cablehead re Twisted I can have a look. I used that like 10 years ago and the implementation will probably be pretty similar to aio. I have never used Tornado in any way, so if you know someone who does a contribution would be great.
Status update on this PR. I plan to add some tests that actually start a consul agent bound to a unix socket and a client connecting to that socket. Something simple like connect && call agent.self(). I want to add these tests for all the different client implementations. Initially this would mean that tests for std and aio would succeed, but tests for twisted and tornado would fail until someone has implemented unix socket support.
@cablehead what do you think? Anything else that could or should be tested?
@cablehead: The introduction of the consul.Consul.from_env() function changes python-consul's current behaviour. Environment variables no longer take precedence over explicitly given parameters. This is related to #192.
IMHO the current behaviour is broken, so by changing this I actually fix a problem. Off course this depends on your point of view. How would you handle this in regards to release and backcompat management?
@asteven I think our two choices are:
- just go for it, and hope no-one is effected
- do one release that keeps the current behavior, but logs a deprecation warning when an environment variable overrides an explicit param. let that sit for a month or two. then push this PR.
I can't think of another option? If you're ok with delaying this PR for a short bit, the latter would be safer for the community...
\cc @matusvalo @abn for input..
I prefer option with one additional release with warnings.
I have quickly reviewed the proposed PR and see my comments. Regarding tornado implementation, here should be code snippet for support of unix sockets:
https://gist.github.com/bdarnell/8641880
I will have a look on it when I will have more time.
Latest checks have failed for reasons that seem unrelated to my changes. Can someone please re-run the tests? If I can do this myself please teach me how.
I have rerun tests and now are green. We have an issue in test suite....
I have created new PR #220 which should fix the issues with unittests. I have already merged this PR to master. @asteven try to update your pull request with newest version of master and we will see if it is better.
Thank you @asteven. Now the PR has green unittests. Currently the PR is missing implementation of unix sockets in Twisted and Tornado backends. Could you finish Twisted backend? I hope I will soon have a look in Tornado.