-
Notifications
You must be signed in to change notification settings - Fork 55
refactor: drop support for etcd v2 API #162
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
lib/resty/etcd.lua
Outdated
local sub_ver = ver.etcdserver:sub(1, 4) | ||
opts.api_prefix = prefix_v3[sub_ver] or "/v3beta" | ||
if not opts.api_prefix or not utils.has_value(prefix_v3, opts.api_prefix) then | ||
local ver, err = etcd_version(opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to detect the version. The only supported prefix now is /v3
, see
lua-resty-etcd/lib/resty/etcd.lua
Line 9 in 70b648f
["3.4."] = "/v3", |
change t/v3/tls.t test case 2 response body form
to
Because the step of getting the version is omitted, the lua-resty-etcd/lib/resty/etcd/v3.lua Lines 140 to 143 in 70b648f
with |
@@ -56,7 +56,7 @@ __DATA__ | |||
local etcd, err = require "resty.etcd" .new({ | |||
protocol = "v3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we only support v3
protocol, can we drop protocol
field? it is useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe etcd will have a v4 protocol in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for protocol
field, the default value should be v3
, all right?
@membphis pls review again, thanks |
etcd will soon drop the V2 API: etcd-io/etcd#12913