jplana / python-etcd

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

Update DNS discovery to better match ETCD documentation. #242

Closed sebastienc closed 10 months ago

sebastienc commented 7 years ago

Hi,

I've made a little change to better match the documentation available here. It will now search for etcd-client.tcp.domain instead of etcd.tcp.domain. I did not had time to implement searching for _etcd-client-ssl first and if there's an error, search for _etcd-client although I've exposed a flag to use it if people want to.

Thanks for the work.

Sebastien

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.4%) to 87.855% when pulling 209661318fa817ceb6409d9d66e3f5eb1c93d049 on sebastienc:dns_discovery into 18c75196ffd04b3104f3ba44356ec1fcbcc4d09a on jplana:master.

sebastienc commented 7 years ago

Alright, I'll make the changes.

On Sat, Jun 17, 2017 at 06:12 Giuseppe Lavagetto notifications@github.com wrote:

@lavagetto requested changes on this pull request.

Thanks for the patch, but I think it needs some restructuring.

In src/etcd/client.py https://github.com/jplana/python-etcd/pull/242#discussion_r122565702:

@@ -65,7 +65,8 @@ def init( use_proxies=False, expected_cluster_id=None, per_host_pool_size=10,

  • lock_prefix="/_locks"
  • lock_prefix="/_locks",
  • srv_use_ssl=False

we already have 'protocol' that can be used instead of a new parameter

In src/etcd/client.py https://github.com/jplana/python-etcd/pull/242#discussion_r122565748:

@@ -223,8 +226,11 @@ def _set_version_info(self): self._version = version_info['etcdserver'] self._cluster_version = version_info['etcdcluster']

  • def _discover(self, domain):
  • srv_name = "_etcd._tcp.{}".format(domain)
  • def _discover(self, domain, use_ssl=False):

I don't like this change as it breaks compatibility with the previous version.

I would make discovery first search for the new alias, then fallback to the old one if discovery returns nothing.

Historically we had client SRV support before the official client, and we went with the same key that confd is using. I still see a lot of people might want to preserve that old name. It should be fairly easy to implement too.

Also, since we save protocol in self.protocol, use that instead of adding a parameter.

— 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/242#pullrequestreview-44697511, or mute the thread https://github.com/notifications/unsubscribe-auth/ABe084qubhqHSFp542fj7G-oDvbuOf4Fks5sE6ZzgaJpZM4NrsxQ .

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.2%) to 87.905% when pulling 7e859ab4bb194a8f5d9cc2f1bff6b8241bf2e36d on sebastienc:dns_discovery into 001d85ebdebd12317a999a70284f1a89a09fea79 on jplana:master.

sebastienc commented 7 years ago

Hi, is this better?