search

crossbario / txaio-etcd

A Twisted client for etcd3
MIT License
14 stars 6 forks source link

Correct OpGet and OpSet options and introduce ignore_value and _lease #8

Closed bra-fsn closed 7 years ago

bra-fsn commented 7 years ago

OpSet.

oberstet commented 7 years ago

Ok, this PR combines a bug fix (straight forward to merge) with an API change (which needs discussion .. I'm not sure I get the use case/necessity).

oberstet commented 7 years ago

Ah, sorry, so etcd itself already has these:

?

bra-fsn commented 7 years ago

Yes, see: https://github.com/coreos/etcd/blob/master/etcdserver/etcdserverpb/rpc.proto#L415

oberstet commented 7 years ago

This doesnt seem to match https://github.com/crossbario/txaio-etcd/blob/master/docs/rpc.swagger.json#L1809

oberstet commented 7 years ago

I copied above file from etcd 3.1 .. did they extend it recently?

Also: this library is using the HTTP/gRPC gateway .. so maybe it is exposed in gRPC but not HTTP?

bra-fsn commented 7 years ago

Hm, you may be right. I've committed a new revision. Should I close this and file a new pull request?

oberstet commented 7 years ago

would be nice, as then we don't have cruft in the history. but I'm not insisting on this.

what you could add in the PR: make sure all the __str__ of the Op derived classes actually spit out all attributes (which makes debugging easier). and double check that we now finally have it right vs rpc.swagger.json;) I was a little sloppy ..

rgd HTTP GW in etcd missing stuff that gRPC native API has: if you care, maybe file a bug with etcd so they are aware ..

bra-fsn commented 7 years ago

Closing this one and opening a new.

oberstet commented 7 years ago

@bra-fsn great!

ah, and if you file a bug with etcd, pls also file a bug here so we remember that we still need to fix it when etcd has fixed it (= exposed in the HTTP GW)

bra-fsn commented 7 years ago

No, I'm fine with the current state. :) I just wanted to be nice, but I don't (yet) need this function.