search

jplana / python-etcd

A python client for etcd
Other
522 stars 210 forks source link

Add proxy support #212

Closed bahner closed 1 year ago

bahner commented 7 years ago

Hepp!

I added this as I needed to use etcd through a proxy. It works well and have been for weeks. Please pull this :-)

Kind regards, bahner

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 88.305% when pulling f31e768365c7d62ba2f2814c36017a4eee4f2321 on bahner:master into 0d0145f5e835aa032c97a0a5e09c4c68b7a03f66 on jplana:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 88.305% when pulling f31e768365c7d62ba2f2814c36017a4eee4f2321 on bahner:master into 0d0145f5e835aa032c97a0a5e09c4c68b7a03f66 on jplana:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.9%) to 87.582% when pulling 757b27cefa716150ce10440732a49e878f2d45f3 on bahner:master into 0d0145f5e835aa032c97a0a5e09c4c68b7a03f66 on jplana:master.

bahner commented 7 years ago

@ashcrow _log statement added. The travis error seems to be unrelated to the actual code submitted.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.6%) to 87.843% when pulling 49810aa0221aa5b0085325c19fe81f2bf84f145e on bahner:master into 0d0145f5e835aa032c97a0a5e09c4c68b7a03f66 on jplana:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.2%) to 87.275% when pulling 72d836c1d3a266e12d0a8cd3f605d82c66be63d4 on bahner:master into 0d0145f5e835aa032c97a0a5e09c4c68b7a03f66 on jplana:master.

lavagetto commented 7 years ago

Please add tests for this (both unit tests and possibly integration?). I am not ok with decreasing our code coverage by 1.2% for a function that is surely ok to have but not really a must-have.

bahner commented 7 years ago

I will add the tests. But don't have time this weekend. :-) The reason for this, as where I work, is that the only allowed access to etcd is through a webproxy. Getting an etcd-proxy between zones is not an alternative - even if this would be cleaner.

The smaller change, with only the proxies, works very well for us, but when I tried to run an etcd locally, not so much.

I will write the tests, but I definetly think this should be in here. Proxies are generaly handled very badly in many applications, and I agree the reading an parsing of os vars is less than perfect. But that's basically the way it is.

Another option would be to write a small module, that does the parsing and hides the complexity somewhat. But that's another discussion.

2017-02-12 13:15 GMT+01:00 Giuseppe Lavagetto notifications@github.com:

@lavagetto requested changes on this pull request.

I think this functionality should live outside of the client (detecting OS variables and so on), and that the client should only get a proxy address if the proxy should be called, and maybe a list of no-proxy machines. But I am honestly doubtful this is useful at all, given you can use the etcd proxy that we support already and is thought as an alternative to a generic web proxy.

Also, code needs to be separated from the init function and be sent. And please add tests!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jplana/python-etcd/pull/212#pullrequestreview-21405945, or mute the thread https://github.com/notifications/unsubscribe-auth/AJq5L02F6VDrhHAR2EeRBfxW9JVvlNvfks5rbvf-gaJpZM4K9Bds .

-- Mvh, Lars Bahner