You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/10/12 01:17:06 UTC
[GitHub] [apisix] RocFang opened a new pull request #2390: fix: it does'nt make sence to compare etcd_version, since it's nil.
RocFang opened a new pull request #2390:
URL: https://github.com/apache/apisix/pull/2390
### What this PR does / why we need it:
1. the result below will always be false ,since etcd_version is nil:
```lua
if (etcd_version == "v3" and not res:find("OK", 1, true)) then
is_success = false
if (index == host_count) then
error(cmd .. "\n" .. res)
end
break
end
```
and since etcd_version has been checked before, I think maybe there is no need to check it again here.
2. cluster_version has been defined previously, no need to define a new local one.
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] RocFang commented on pull request #2390: fix: it does'nt make sence to compare etcd_version, since it's nil.
Posted by GitBox <gi...@apache.org>.
RocFang commented on pull request #2390:
URL: https://github.com/apache/apisix/pull/2390#issuecomment-707792636
@membphis BTW, I think bin/apisix should also be checked by luacheck, and then the little mistake like this undifinend var in comparasion expression will not happen. Only problem is the constraint of line length may be different now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2390: fix: it does'nt make sence to compare etcd_version, since it's nil.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2390:
URL: https://github.com/apache/apisix/pull/2390#issuecomment-707863641
@RocFang many thx
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu merged pull request #2390: fix: it does'nt make sence to compare etcd_version, since it's nil.
Posted by GitBox <gi...@apache.org>.
Yiyiyimu merged pull request #2390:
URL: https://github.com/apache/apisix/pull/2390
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2390: fix: it does'nt make sence to compare etcd_version, since it's nil.
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2390:
URL: https://github.com/apache/apisix/pull/2390#issuecomment-707801650
> @membphis BTW, I think bin/apisix should also be checked by luacheck, and then the little mistake like this undifinend var in comparasion expression will not happen. Only problem is the constraint of line length may be different now.
That's a good idea! Actually tokers is fixing it in #2395. Welcome review~
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2390: fix: it does'nt make sence to compare etcd_version, since it's nil.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2390:
URL: https://github.com/apache/apisix/pull/2390#discussion_r503986464
##########
File path: bin/apisix
##########
@@ -1000,7 +1000,7 @@ local function init_etcd(show_output)
os.exit(1)
end
- local cluster_version = body["etcdcluster"]
+ cluster_version = body["etcdcluster"]
Review comment:
agree +1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2390: fix: it does'nt make sence to compare etcd_version, since it's nil.
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2390:
URL: https://github.com/apache/apisix/pull/2390#discussion_r503979530
##########
File path: bin/apisix
##########
@@ -1000,7 +1000,7 @@ local function init_etcd(show_output)
os.exit(1)
end
- local cluster_version = body["etcdcluster"]
+ cluster_version = body["etcdcluster"]
Review comment:
Maybe delete the outside definition is better, since we use the new one in each cycle.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] RocFang commented on a change in pull request #2390: fix: it does'nt make sence to compare etcd_version, since it's nil.
Posted by GitBox <gi...@apache.org>.
RocFang commented on a change in pull request #2390:
URL: https://github.com/apache/apisix/pull/2390#discussion_r504004215
##########
File path: bin/apisix
##########
@@ -1000,7 +1000,7 @@ local function init_etcd(show_output)
os.exit(1)
end
- local cluster_version = body["etcdcluster"]
+ cluster_version = body["etcdcluster"]
Review comment:
@Yiyiyimu @membphis updated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org