Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for etcd client version 3 #2

Merged
merged 7 commits into from
Apr 23, 2019
Merged

Conversation

khvysofq
Copy link
Contributor

Hi, kpacha. This PR is primarily to support the functionality of the ETCD clientv3. The major changes are as follows

  • Add clientv3.goclientv3_testing.go in the root of the source code.
  • Move some common parts into the config.go

Both modes are supported through profile configuration, just add "client_version": "v3" option in extra_config

"extra_config": {
    "github.com/devopsfaith/krakend-etcd": {
        "machines": [ "http://192.168.110.111:2379" ],
		"client_version": "v3",
        "options": {
            "dial_timeout": "5s",
            "dial_keepalive": "30s",
            "header_timeout": "1s"
        }
    }
}
  • "client_version": "v3" using the etcd clientv3
  • "client_version": "v2" using the etcd clientv2 (old version)

The default value is v2, if you are not set "client_version". I have tested in my local machines.


Among other things, there is one small problem. I used go mod to manage my go packages. but in the original code of the example/main.go, the github.com/devopsfaith/krakend-etcd(v 3.3.10 or newer version) and github.com/devopsfaith/krakend-viper conflict when load package github.com/ugorji/go

  • github.com/devopsfaith/krakend-etcd(v 3.3.12) need github.com/ugorji/[email protected]
  • github.com/devopsfaith/krakend-viper auto need github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8

They need the same package, but not the same directory and not the same version. The go mod can't handle this situation. see spf13/viper#658

So, in the example/main.go, I can only change github.com/devopsfaith/krakend-viper to github.com/devopsfaith/krakend/config

@kpacha kpacha self-assigned this Apr 21, 2019
@kpacha kpacha added this to the 0.10 milestone Apr 21, 2019
Copy link
Contributor

@kpacha kpacha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great contribution!

Please, check my comments

config.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
clientv3.go Outdated Show resolved Hide resolved
example/krakend.json Outdated Show resolved Hide resolved
example/main.go Outdated Show resolved Hide resolved
subscriber.go Outdated Show resolved Hide resolved
@khvysofq
Copy link
Contributor Author

HI, thanks for you PR review, I have solved, See the commits.

config.go Outdated Show resolved Hide resolved
clientv3.go Outdated Show resolved Hide resolved
@kpacha
Copy link
Contributor

kpacha commented Apr 22, 2019

@khvysofq I've added two more comments regarding two minor details and I think it's almost ready to be merged.

Again, thanks for the contribution to the project

@khvysofq
Copy link
Contributor Author

The KrakenD is a very promising project . I am honored to contribute to this project. Anyway if you have any suggestions for this PR, please point it out. I want do it better as krakenD project do.

@kpacha kpacha merged commit 4418916 into devopsfaith:master Apr 23, 2019
@kpacha kpacha mentioned this pull request Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants