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 2021/07/14 13:52:19 UTC
[GitHub] [apisix] RocFang opened a new pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
RocFang opened a new pull request #4606:
URL: https://github.com/apache/apisix/pull/4606
…ent has no chance to be nil or false
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
correct the return value checking of pcall
<!--- If it fixes an open issue, please link to the issue here. -->
### 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? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] RocFang commented on a change in pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
RocFang commented on a change in pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#discussion_r670555324
##########
File path: apisix/plugins/client-control.lua
##########
@@ -48,7 +48,7 @@ end
function _M.rewrite(conf, ctx)
- if not apisix_ngx_client then
+ if not ok then
Review comment:
@tokers maybe it's another issue? :)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #4606:
URL: https://github.com/apache/apisix/pull/4606
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#discussion_r670110395
##########
File path: apisix/plugins/client-control.lua
##########
@@ -48,7 +48,7 @@ end
function _M.rewrite(conf, ctx)
- if not apisix_ngx_client then
+ if not ok then
Review comment:
The module is loaded by default, and "resty.apisix.client" is only included in APISIX-OpenResty. So showing an error message will annoy people who use regular OpenResty.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] RocFang edited a comment on pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
RocFang edited a comment on pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#issuecomment-879938949
emm.. I think the ci failure has nothing to do with my pr:)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] RocFang commented on a change in pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
RocFang commented on a change in pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#discussion_r670103118
##########
File path: apisix/plugins/client-control.lua
##########
@@ -48,7 +48,7 @@ end
function _M.rewrite(conf, ctx)
- if not apisix_ngx_client then
+ if not ok then
Review comment:
@tokers thx. i guess you mean throw an error when failed calling pcall, so this plugin will not be loaded. I agree with that actually, but only try to make the patch as small as possbile.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#discussion_r670365129
##########
File path: apisix/plugins/client-control.lua
##########
@@ -48,7 +48,7 @@ end
function _M.rewrite(conf, ctx)
- if not apisix_ngx_client then
+ if not ok then
Review comment:
But from the HTTP semantic, maybe `501` is better in such a case (than `503`).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#discussion_r670047641
##########
File path: apisix/plugins/client-control.lua
##########
@@ -48,7 +48,7 @@ end
function _M.rewrite(conf, ctx)
- if not apisix_ngx_client then
+ if not ok then
Review comment:
Is this better to show the error message while APISIX is starting?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#discussion_r670915975
##########
File path: apisix/plugins/client-control.lua
##########
@@ -48,7 +48,7 @@ end
function _M.rewrite(conf, ctx)
- if not apisix_ngx_client then
+ if not ok then
Review comment:
Got it.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#discussion_r670364575
##########
File path: apisix/plugins/client-control.lua
##########
@@ -48,7 +48,7 @@ end
function _M.rewrite(conf, ctx)
- if not apisix_ngx_client then
+ if not ok then
Review comment:
OK, got it.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] RocFang commented on pull request #4606: fix: correct the return value checking of pcall, since apisix_ngx_cli…
Posted by GitBox <gi...@apache.org>.
RocFang commented on pull request #4606:
URL: https://github.com/apache/apisix/pull/4606#issuecomment-879938949
emm.. I thank the ci failure has nothing to do with my pr:)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org