Add etcd v3 api support #4
Conversation
store/etcdv3/etcdv3.go
Outdated
| if options.Username != "" { | ||
| setCredentials(cfg, options.Username, options.Password) | ||
| } | ||
| if options.SyncPeriod != 0 { |
There was a problem hiding this comment.
SyncPeriod is not in docker/libkv/store.Config. It's only in your fork. The import statement should be changed IMHO.
There was a problem hiding this comment.
Thanks, you're right. I'm still working on the main repository, pushing to my remote. I don't like renaming import paths (kinda dirty), but I don't think these features will end up being included upstream anyways. So I'll have to do it eventually 😞
(I also thought about creating a new repository with a new name to clearly mark a separation from the upstream libkv, I'm not sure yet)
There was a problem hiding this comment.
Oh and another solution: people could still reference the fork with their dependency manager (glide, etc.).
It's still one more step to contribute though: you have to add my remote and rebase on top of my master branch before submitting a PR with a feature/patch to the fork. I can write some docs for this though.
There was a problem hiding this comment.
I also thought about creating a new repository with a new name to clearly mark a separation from the upstream libkv, I'm not sure yet
From my POV that would be the best solution. I would be interested in contributing. Right now it's not clear if it's temporary fork, if the changes will be merged to upstream repo. Based on the number of issues in upstream repo looks like there is high demand for that lib and for improving it.
Oh and another solution: people could still reference the fork with their dependency manager (glide, etc.).
That's true but only if you use package managers that support that. I've recently migrated to dep because of significant speed improvements and dep doesn't have that feature.
There was a problem hiding this comment.
Thank you for the feedback. I'll certainly take that into account. I will keep on working on it and prepare for a rename if needs be. I have no intention to contribute to the upstream repo because it does not seem active anymore, so this fork is actually where the work will be happening. I guess a rename makes sense in this case.
There was a problem hiding this comment.
One question. Why have you disabled Issues in your fork?
There was a problem hiding this comment.
I haven't disabled them, I forgot to turn them on 🤣 I thought they were enabled by default on forks somehow, seems not. Thanks for pointing that out.
Signed-off-by: Alexandre Beslic <abeslic@abronan.com>
Signed-off-by: Alexandre Beslic <abeslic@abronan.com>
|
What is needed in order for this to go back to the original project? |
Initial support for etcd v3 API.
Signed-off-by: Alexandre Beslic abeslic@abronan.com