You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Revolyssup (via GitHub)" <gi...@apache.org> on 2023/05/31 09:19:26 UTC
[GitHub] [apisix] Revolyssup opened a new pull request, #9580: [draft]fix: Improve results from admin API while GET all plugins
Revolyssup opened a new pull request, #9580:
URL: https://github.com/apache/apisix/pull/9580
### Description
- When subsystem is wrong, return a 400 error.
- When subsystem is unspecified and all is true, return all plugins and not just http plugins
- Test case to be added.
Fixes #9577
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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] Revolyssup commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1581987524
Current state of this PR reflects this proposal: https://github.com/apache/apisix/issues/9577#issuecomment-1581984384
--
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] kayx23 commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1579716589
> Should we change the default behaviour
Breaking changes aside:
1. If by `default behaviour` you mean to return all plugins when `subsystem` is not specified after `all=true`, and use `subsystem` for filtering purpose, personally I think this makes the most sense.
2. Note that `all=true` seems to be a fixed pre-req for using `subsystem`. If I set it to false, it errors out:
```
curl -s "http://127.0.0.1:9180/apisix/admin/plugins?all=false"
{"error_msg":"not found plugin name"}
```
But does this make sense? Why is this needed at all?
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266214618
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,15 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
Review Comment:
done
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,20 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
- end
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
Review Comment:
done
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254152567
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,15 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- arg all to be deprecated
+ if (arg and arg["all"] == "true") or not name then
Review Comment:
```suggestion
if (arg and arg["all"] == "true") then
```
There is no need to change the original condition, because it will be deprecated
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219794124
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
Review Comment:
done
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
+ return 400, {error_msg = "no plugin found for subsystem: "..subsystem}
+ end
Review Comment:
done
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
+ return 400, {error_msg = "no plugin found for subsystem: "..subsystem}
Review Comment:
done
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219794486
##########
apisix/admin/plugins.lua:
##########
@@ -53,11 +58,20 @@ function _M.get(name)
scope = true,
})
- if arg["subsystem"] == "stream" then
+ if subsystem == "stream" then
return 200, stream_plugins
end
-
- return 200, http_plugins
+ if subsystem == "http" then
+ return 200, http_plugins
+ end
+ local allPlugins = {}
Review Comment:
done
##########
apisix/admin/plugins.lua:
##########
@@ -53,11 +58,20 @@ function _M.get(name)
scope = true,
})
- if arg["subsystem"] == "stream" then
+ if subsystem == "stream" then
return 200, stream_plugins
end
-
- return 200, http_plugins
+ if subsystem == "http" then
Review Comment:
done
##########
apisix/admin/plugins.lua:
##########
@@ -53,11 +58,20 @@ function _M.get(name)
scope = true,
})
- if arg["subsystem"] == "stream" then
+ if subsystem == "stream" then
return 200, stream_plugins
end
-
- return 200, http_plugins
+ if subsystem == "http" then
+ return 200, http_plugins
+ end
+ local allPlugins = {}
+ for k, v in pairs(http_plugins) do
+ allPlugins[k] = v
+ end
+ for k, v in pairs(stream_plugins) do
+ allPlugins[k] = v
+ end
Review Comment:
done
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264703689
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ end
+
local priorities = {}
local success = {}
- for i, name in ipairs(plugins) do
- local plugin_name = "apisix.plugins." .. name
- local ok, plugin = pcall(require, plugin_name)
- if ok and plugin.priority then
- priorities[name] = plugin.priority
- table_insert(success, name)
+ if http_plugins then
+ for i, name in ipairs(http_plugins) do
+ local plugin_name = "apisix.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
Review Comment:
Those functions are to get the entire plugin loaded with plugin.load(). Here we just need the names.
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ end
+
local priorities = {}
local success = {}
- for i, name in ipairs(plugins) do
- local plugin_name = "apisix.plugins." .. name
- local ok, plugin = pcall(require, plugin_name)
- if ok and plugin.priority then
- priorities[name] = plugin.priority
- table_insert(success, name)
+ if http_plugins then
+ for i, name in ipairs(http_plugins) do
+ local plugin_name = "apisix.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
+ table_insert(success, name)
+ end
+ end
+ end
+
+ if stream_plugins then
+ for i, name in ipairs(stream_plugins) do
+ local plugin_name = "apisix.stream.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
+ table_insert(success, name)
Review Comment:
Those functions are to get the entire plugin loaded with plugin.load(). Here we just need the names.
--
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] monkeyDluffy6017 commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1610696737
Could you update the doc?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1243278910
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,14 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
Review Comment:
This method should be kept. Let's add WARNING log about it will be deprecated soon
--
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] Revolyssup commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1614155680
Approve CI please
--
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] Revolyssup commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1577271157
@monkeyDluffy6017 the failing tests are unrelated to the PR. Can you rerun CI? These tests are flaky
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1244642743
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,30 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through all subsystems
+ local all_subsystem = true
+ if subsystem then
+ all_subsystem = false
+ end
+
+ -- When subsystem is passed as http or not passed at all(searching through all)
+ if all_subsystem or subsystem == "http" then
+ plugin = plugin_get_http(name)
end
Review Comment:
It's a bit complicated here, can we just make it simple like this
```
if subsystem == "stream" then
plugin = plugin_get_stream(name)
else
plugin = plugin_get_http(name)
end
```
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1243329713
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,14 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- When name is unspecified, return all plugins for the given subsystem
Review Comment:
Why add this logic, it is not in the proposal
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1243329498
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,14 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
Review Comment:
I added the warning log @monkeyDluffy6017
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263547127
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,23 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
+
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
end
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "stream" then
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
+ if not plugin then
+ subsystem = subsystem or "http"
Review Comment:
This logic is duplicated with https://github.com/apache/apisix/pull/9580/files#diff-00090cde519e7280adbc73e830b92089c4e62a22057c1b7577c77dc1456e72e5R75
--
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] monkeyDluffy6017 merged pull request #9580: change: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #9580:
URL: https://github.com/apache/apisix/pull/9580
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263555124
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,23 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
+
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
end
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "stream" then
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
+ if not plugin then
+ subsystem = subsystem or "http"
+ local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
core.log.warn("failed to load plugin [", name, "] err: ", plugin)
- return 400, {error_msg = "failed to load plugin " .. name}
+ return 400, {error_msg = err }
Review Comment:
I think there is no need to return an error here?
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1244709847
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,30 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through all subsystems
+ local all_subsystem = true
+ if subsystem then
+ all_subsystem = false
+ end
+
+ -- When subsystem is passed as http or not passed at all(searching through all)
+ if all_subsystem or subsystem == "http" then
+ plugin = plugin_get_http(name)
end
- local plugin_name = "apisix.plugins." .. name
+ -- When subsystem is passed as stream or not passed at all(searching through all)
+ if (not plugin) and (all_subsystem or subsystem == "stream") then
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
+ if not plugin then
+ local err = "failed to load plugin " .. name
+ if subsystem then
+ err = err .. " in subsystem ".. subsystem
+ end
Review Comment:
For /{name} , the default isn't http. It will be searched in all subsystems unless explicitly specified. That was part of the proposal. To make things explicit.
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254196253
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,15 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- arg all to be deprecated
+ if (arg and arg["all"] == "true") or not name then
Review Comment:
`not name` is basically like passing `/apisix/admin/plugins`
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263551492
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,23 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
+
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
end
Review Comment:
```suggestion
if subsystem == "http" then
plugin = plugin_get_http(name)
else
plugin = plugin_get_stream(name)
end
```
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263568078
##########
docs/en/latest/admin-api.md:
##########
@@ -1339,26 +1347,10 @@ curl "http://127.0.0.1:9180/apisix/admin/plugins/key-auth" -H 'X-API-KEY: ed
:::tip
-You can use the `/apisix/admin/plugins?all=true` API to get all properties of all plugins.
-
-Each Plugin has the attributes `name`, `priority`, `type`, `schema`, `consumer_schema` and `version`.
-
-Defaults to only L7 Plugins. If you need to get attributes of L4 / Stream Plugins, use `/apisix/admin/plugins?all=true&subsystem=stream`.
+You can use the `/apisix/admin/plugins?all=true` API to get all properties of all plugins. The query param `?all=true` will be deprecated soon.
Review Comment:
```suggestion
You can use the `/apisix/admin/plugins?all=true` API to get all properties of all plugins. This API will be deprecated soon.
```
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263571994
##########
docs/zh/latest/admin-api.md:
##########
@@ -1358,7 +1366,7 @@ Plugin 资源请求地址：/apisix/admin/plugins/{plugin_name}
你可以使用 `/apisix/admin/plugins?all=true` 接口获取所有插件的所有属性，每个插件包括 `name`，`priority`，`type`，`schema`，`consumer_schema` 和 `version`。
-默认情况下，该接口只返回 L7 插件。如果你需要获取 L4 / Stream 插件，需要使用 `/apisix/admin/plugins?all=true&subsystem=stream`。
+您可以使用“/apisix/admin/plugins?all=true”API 获取所有插件的所有属性。查询参数 `?all=true` 将很快被弃用。
Review Comment:
Delete the whole tip, use the following one intead:
```
您可以使用 “/apisix/admin/plugins?all=true” 获取所有插件的所有属性。这个 API 将很快被弃用
```
##########
docs/zh/latest/admin-api.md:
##########
@@ -1358,7 +1366,7 @@ Plugin 资源请求地址：/apisix/admin/plugins/{plugin_name}
你可以使用 `/apisix/admin/plugins?all=true` 接口获取所有插件的所有属性，每个插件包括 `name`，`priority`，`type`，`schema`，`consumer_schema` 和 `version`。
-默认情况下，该接口只返回 L7 插件。如果你需要获取 L4 / Stream 插件，需要使用 `/apisix/admin/plugins?all=true&subsystem=stream`。
+您可以使用“/apisix/admin/plugins?all=true”API 获取所有插件的所有属性。查询参数 `?all=true` 将很快被弃用。
Review Comment:
Delete the whole tip, use the following one intead:
```
您可以使用 “/apisix/admin/plugins?all=true” 获取所有插件的所有属性。这个 API 将很快被弃用
```
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264704135
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
Review Comment:
`subsystem` is being checked here - https://github.com/apache/apisix/pull/9580/files#diff-00090cde519e7280adbc73e830b92089c4e62a22057c1b7577c77dc1456e72e5R108
I didn't get you.
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219625066
##########
apisix/admin/plugins.lua:
##########
@@ -53,11 +58,20 @@ function _M.get(name)
scope = true,
})
- if arg["subsystem"] == "stream" then
+ if subsystem == "stream" then
return 200, stream_plugins
end
-
- return 200, http_plugins
+ if subsystem == "http" then
+ return 200, http_plugins
+ end
+ local allPlugins = {}
Review Comment:
Add a blank
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219595831
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
Review Comment:
Why do you check `subsystem~=""`? It sholud be only `http` and `stream` and `nil`
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266233921
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ end
+
local priorities = {}
local success = {}
- for i, name in ipairs(plugins) do
- local plugin_name = "apisix.plugins." .. name
- local ok, plugin = pcall(require, plugin_name)
- if ok and plugin.priority then
- priorities[name] = plugin.priority
- table_insert(success, name)
+ if http_plugins then
+ for i, name in ipairs(http_plugins) do
+ local plugin_name = "apisix.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
Review Comment:
done
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ end
+
local priorities = {}
local success = {}
- for i, name in ipairs(plugins) do
- local plugin_name = "apisix.plugins." .. name
- local ok, plugin = pcall(require, plugin_name)
- if ok and plugin.priority then
- priorities[name] = plugin.priority
- table_insert(success, name)
+ if http_plugins then
+ for i, name in ipairs(http_plugins) do
+ local plugin_name = "apisix.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
+ table_insert(success, name)
+ end
+ end
+ end
+
+ if stream_plugins then
+ for i, name in ipairs(stream_plugins) do
+ local plugin_name = "apisix.stream.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
+ table_insert(success, name)
Review Comment:
done
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264703981
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,23 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
+
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
end
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "stream" then
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
+ if not plugin then
+ subsystem = subsystem or "http"
Review Comment:
removed
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219595831
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
Review Comment:
Why do you check `subsystem~=""`? It sholud be only "http" and "stream" and nil
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219626071
##########
apisix/admin/plugins.lua:
##########
@@ -53,11 +58,20 @@ function _M.get(name)
scope = true,
})
- if arg["subsystem"] == "stream" then
+ if subsystem == "stream" then
return 200, stream_plugins
end
-
- return 200, http_plugins
+ if subsystem == "http" then
+ return 200, http_plugins
+ end
+ local allPlugins = {}
Review Comment:
please use the snake_case style: `all_plugins`
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1244616541
##########
apisix/admin/plugins.lua:
##########
@@ -52,24 +62,37 @@ function _M.get(name)
type = true,
scope = true,
})
-
- if arg["subsystem"] == "stream" then
- return 200, stream_plugins
+ local all_plugins = http_plugins
+ for k, v in pairs(stream_plugins) do
+ all_plugins[k] = v
end
+ return 200, all_plugins
Review Comment:
We don't need to change this, the api is going to be deprecated, so just leave it as it was
##########
apisix/admin/plugins.lua:
##########
@@ -52,24 +62,37 @@ function _M.get(name)
type = true,
scope = true,
})
-
- if arg["subsystem"] == "stream" then
- return 200, stream_plugins
+ local all_plugins = http_plugins
+ for k, v in pairs(stream_plugins) do
+ all_plugins[k] = v
end
+ return 200, all_plugins
Review Comment:
We don't need to change this, the API is going to be deprecated, so just leave it as it was
--
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] monkeyDluffy6017 commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1610687217
Does the API "/apisix/admin/plugins/list" support the parameter "subsystem" ?
--
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] Revolyssup commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1610746500
> Does the API "/apisix/admin/plugins/list" support the parameter "subsystem" ?
No
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1243333119
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,14 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- When name is unspecified, return all plugins for the given subsystem
Review Comment:
This is present in the proposal document on 3.2.
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1243434926
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,14 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- When name is unspecified, return all plugins for the given subsystem
Review Comment:
The full schema is no needed any more
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1213853876
##########
apisix/admin/plugins.lua:
##########
@@ -25,7 +25,6 @@ local get_uri_args = ngx.req.get_uri_args
local plugin_get_all = require("apisix.plugin").get_all
local encrypt_conf = require("apisix.plugin").encrypt_conf
local pairs = pairs
-
Review Comment:
No need to remove this
--
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] Revolyssup commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1610771571
> Could you update the doc?
@monkeyDluffy6017 I updated the doc.
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254167305
##########
apisix/admin/init.lua:
##########
@@ -253,8 +254,9 @@ end
local function get_plugins_list()
set_ctx_and_check_token()
-
- local plugins = resources.plugins.get_plugins_list()
+ local args = get_uri_args()
+ local subsystem = args["subsystem"]
+ local plugins = resources.plugins.get_plugins_list(subsystem)
Review Comment:
`htt&stream` is useless, so the value could be `http`, `stream`, `nil`
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254122474
##########
apisix/admin/init.lua:
##########
@@ -253,8 +254,9 @@ end
local function get_plugins_list()
set_ctx_and_check_token()
-
- local plugins = resources.plugins.get_plugins_list()
+ local args = get_uri_args()
+ local subsystem = args["subsystem"]
+ local plugins = resources.plugins.get_plugins_list(subsystem)
Review Comment:
Should we check the value of subsystem?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254164206
##########
apisix/admin/plugins.lua:
##########
@@ -85,8 +106,14 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local plugins
+ if subsystem == "stream" then
+ plugins = core.config.local_conf().stream_plugins
+ else
+ plugins = core.config.local_conf().plugins
+ end
+
Review Comment:
apisix.stream.plugins is the directory of stream plugins
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254167305
##########
apisix/admin/init.lua:
##########
@@ -253,8 +254,9 @@ end
local function get_plugins_list()
set_ctx_and_check_token()
-
- local plugins = resources.plugins.get_plugins_list()
+ local args = get_uri_args()
+ local subsystem = args["subsystem"]
+ local plugins = resources.plugins.get_plugins_list(subsystem)
Review Comment:
htt&stream could not be accepted, it's not useless
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254192393
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,15 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- arg all to be deprecated
+ if (arg and arg["all"] == "true") or not name then
Review Comment:
@monkeyDluffy6017 This condition is so that `/plugins` can also return what `/plugins?all=true` returns currently. Adding this or condition is important to make the behaviour of `/plugins` == behaviour of `plugins?all=true`
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254242049
##########
apisix/admin/init.lua:
##########
@@ -253,8 +254,9 @@ end
local function get_plugins_list()
set_ctx_and_check_token()
-
- local plugins = resources.plugins.get_plugins_list()
+ local args = get_uri_args()
+ local subsystem = args["subsystem"]
+ local plugins = resources.plugins.get_plugins_list(subsystem)
Review Comment:
done
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264701101
##########
docs/zh/latest/admin-api.md:
##########
@@ -1358,7 +1366,7 @@ Plugin 资源请求地址：/apisix/admin/plugins/{plugin_name}
你可以使用 `/apisix/admin/plugins?all=true` 接口获取所有插件的所有属性，每个插件包括 `name`，`priority`，`type`，`schema`，`consumer_schema` 和 `version`。
-默认情况下，该接口只返回 L7 插件。如果你需要获取 L4 / Stream 插件，需要使用 `/apisix/admin/plugins?all=true&subsystem=stream`。
+您可以使用“/apisix/admin/plugins?all=true”API 获取所有插件的所有属性。查询参数 `?all=true` 将很快被弃用。
Review Comment:
done
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266107295
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,20 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
- end
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
Review Comment:
delete
```
subsystem = subsystem or "http"
```
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266218101
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
Review Comment:
@monkeyDluffy6017 The check is already performed here https://github.com/apache/apisix/pull/9580/files#diff-8baac20135bcefd8d1beffe08f44b5271bc1c717c6ab2e720a0e14cb1a02599dR262 before calling this function
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266278752
##########
Makefile:
##########
@@ -403,7 +403,7 @@ uninstall:
test: runtime
@$(call func_echo_status, "$@ -> [ Start ]")
$(ENV_GIT) submodule update --init --recursive
- prove -I../test-nginx/lib -I./ -r -s t/
+ prove -I../test-nginx/lib -I./ -r -s t/admin/plugins.t
Review Comment:
Why do you add this?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219588495
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
Review Comment:
```suggestion
if subsystem and subsystem ~= "" and subsystem ~= "http" and subsystem ~= "stream" then
```
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219617308
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
+ return 400, {error_msg = "no plugin found for subsystem: "..subsystem}
Review Comment:
`return 400, {error_msg = "unsupported subsystem: "..subsystem}`
Is it better?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219607640
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
+ return 400, {error_msg = "no plugin found for subsystem: "..subsystem}
+ end
Review Comment:
Add a blank below
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1244686769
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,30 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through all subsystems
+ local all_subsystem = true
+ if subsystem then
+ all_subsystem = false
+ end
+
+ -- When subsystem is passed as http or not passed at all(searching through all)
+ if all_subsystem or subsystem == "http" then
+ plugin = plugin_get_http(name)
end
Review Comment:
No. The default is to filter through both subsystems unless a specific subsystem is specified via query params. This is written in the proposal as well. To make things explicit.
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1244644877
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,30 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through all subsystems
+ local all_subsystem = true
+ if subsystem then
+ all_subsystem = false
+ end
+
+ -- When subsystem is passed as http or not passed at all(searching through all)
+ if all_subsystem or subsystem == "http" then
+ plugin = plugin_get_http(name)
end
- local plugin_name = "apisix.plugins." .. name
+ -- When subsystem is passed as stream or not passed at all(searching through all)
+ if (not plugin) and (all_subsystem or subsystem == "stream") then
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
+ if not plugin then
+ local err = "failed to load plugin " .. name
+ if subsystem then
+ err = err .. " in subsystem ".. subsystem
+ end
Review Comment:
```suggestion
subsystem = subsystem or "http"
local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
```
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263562092
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ end
+
local priorities = {}
local success = {}
- for i, name in ipairs(plugins) do
- local plugin_name = "apisix.plugins." .. name
- local ok, plugin = pcall(require, plugin_name)
- if ok and plugin.priority then
- priorities[name] = plugin.priority
- table_insert(success, name)
+ if http_plugins then
+ for i, name in ipairs(http_plugins) do
+ local plugin_name = "apisix.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
Review Comment:
can these two function be used here?
```
local plugin_get_http = require("apisix.plugin").get
local plugin_get_stream = require("apisix.plugin").get_stream
```
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ end
+
local priorities = {}
local success = {}
- for i, name in ipairs(plugins) do
- local plugin_name = "apisix.plugins." .. name
- local ok, plugin = pcall(require, plugin_name)
- if ok and plugin.priority then
- priorities[name] = plugin.priority
- table_insert(success, name)
+ if http_plugins then
+ for i, name in ipairs(http_plugins) do
+ local plugin_name = "apisix.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
+ table_insert(success, name)
+ end
+ end
+ end
+
+ if stream_plugins then
+ for i, name in ipairs(stream_plugins) do
+ local plugin_name = "apisix.stream.plugins." .. name
+ local ok, plugin = pcall(require, plugin_name)
+ if ok and plugin.priority then
+ priorities[name] = plugin.priority
+ table_insert(success, name)
Review Comment:
ditto
--
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] monkeyDluffy6017 commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1637251591
Please fix the doc lint
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266118191
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +99,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ else
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+
+
Review Comment:
Redundant blank
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266234172
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,20 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
- end
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
+ else
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
- core.log.warn("failed to load plugin [", name, "] err: ", plugin)
- return 400, {error_msg = "failed to load plugin " .. name}
+ if not plugin then
+ local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
+ core.log.warn("failed to load plugin [", name, "] err: ", err)
+ return 404, {error_msg = "plugin not found"}
Review Comment:
done
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,20 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
- end
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
+ else
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
- core.log.warn("failed to load plugin [", name, "] err: ", plugin)
- return 400, {error_msg = "failed to load plugin " .. name}
+ if not plugin then
+ local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
+ core.log.warn("failed to load plugin [", name, "] err: ", err)
Review Comment:
done
--
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] Revolyssup commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1608849295
> @Revolyssup have you updated the pr based on the proposal?
@monkeyDluffy6017 Yes the PR reflects the state of the proposal that was sent out.
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1244628711
##########
apisix/admin/plugins.lua:
##########
@@ -52,24 +62,37 @@ function _M.get(name)
type = true,
scope = true,
})
-
- if arg["subsystem"] == "stream" then
- return 200, stream_plugins
+ local all_plugins = http_plugins
+ for k, v in pairs(stream_plugins) do
+ all_plugins[k] = v
end
+ return 200, all_plugins
Review Comment:
done. The behaviour of all=true is now as it was previously, with an extra warn log telling that this will be deprecated.
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1244642743
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,30 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through all subsystems
+ local all_subsystem = true
+ if subsystem then
+ all_subsystem = false
+ end
+
+ -- When subsystem is passed as http or not passed at all(searching through all)
+ if all_subsystem or subsystem == "http" then
+ plugin = plugin_get_http(name)
end
Review Comment:
It's a bit complicated here, can we just make it simple like this
```
if subsystem == "stream" then
plugin = plugin_get_stream(name)
else
plugin = plugin_get_http(name)
end
```
The default subsystem is "http", right?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254144083
##########
apisix/admin/plugins.lua:
##########
@@ -85,8 +106,14 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local plugins
+ if subsystem == "stream" then
+ plugins = core.config.local_conf().stream_plugins
+ else
+ plugins = core.config.local_conf().plugins
+ end
+
Review Comment:
The stream plugins is not in the directory `apisix.plugins`
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263563789
##########
docs/en/latest/admin-api.md:
##########
@@ -1318,6 +1318,14 @@ Plugin resource request address: /apisix/admin/plugins/{plugin_name}
The Plugin ({plugin_name}) of the data structure.
+### Request Arguments
+
+| Name | Description | Default |
+| --------- | ----------------------------- | ------- |
+| subsystem | The subsystem of the Plugins. | http |
+
+The plugin can be filtered on subsystem so that the ({plugin_name}) is searched in the subsystem passed through query params. Note that this query parameter is not available on `/apisix/admin/plugins/list`
Review Comment:
this query parameter is not available on `/apisix/admin/plugins/list`?
I think it's supported on `/apisix/admin/plugins/list`?
##########
docs/en/latest/admin-api.md:
##########
@@ -1318,6 +1318,14 @@ Plugin resource request address: /apisix/admin/plugins/{plugin_name}
The Plugin ({plugin_name}) of the data structure.
+### Request Arguments
+
+| Name | Description | Default |
+| --------- | ----------------------------- | ------- |
+| subsystem | The subsystem of the Plugins. | http |
+
+The plugin can be filtered on subsystem so that the ({plugin_name}) is searched in the subsystem passed through query params. Note that this query parameter is not available on `/apisix/admin/plugins/list`
Review Comment:
I think it's supported on `/apisix/admin/plugins/list`?
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264701521
##########
docs/zh/latest/admin-api.md:
##########
@@ -1319,6 +1319,14 @@ Content-Type: text/plain
Plugin 资源请求地址：/apisix/admin/plugins/{plugin_name}
+### 请求参数
+
+| 名称 | 描述 | 默认 |
+| --------- | -------------------------------------- | -------- |
+| subsystem | 插件子系统。 | http |
+
+可以在子系统上过滤插件，以便在通过查询参数传递的子系统中搜索 ({plugin_name})。请注意，此查询参数在“/apisix/admin/plugins/list”上不可用
Review Comment:
It wasn't present present previously and was later added in the pull request. I removed the later comment.
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266108171
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,20 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
- end
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
+ else
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
- core.log.warn("failed to load plugin [", name, "] err: ", plugin)
- return 400, {error_msg = "failed to load plugin " .. name}
+ if not plugin then
+ local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
+ core.log.warn("failed to load plugin [", name, "] err: ", err)
Review Comment:
The msg should be changed
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264703828
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,23 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
+
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
end
Review Comment:
done
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254192393
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,15 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- arg all to be deprecated
+ if (arg and arg["all"] == "true") or not name then
Review Comment:
@monkeyDluffy6017 This condition is so that `/plugins` can also return what `/plugins?all=true` returns currently. Adding this or condition is important to make the behaviour of `/plugins` == behaviour of `plugins?all=true`
--
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] monkeyDluffy6017 commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1578753620
@kayx23 Should we change the default behaviour of this interface? or just add a parameter `subsystem=all` to get all plugins?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219634204
##########
apisix/admin/plugins.lua:
##########
@@ -53,11 +58,20 @@ function _M.get(name)
scope = true,
})
- if arg["subsystem"] == "stream" then
+ if subsystem == "stream" then
return 200, stream_plugins
end
-
- return 200, http_plugins
+ if subsystem == "http" then
+ return 200, http_plugins
+ end
+ local allPlugins = {}
+ for k, v in pairs(http_plugins) do
+ allPlugins[k] = v
+ end
+ for k, v in pairs(stream_plugins) do
+ allPlugins[k] = v
+ end
Review Comment:
local all_plugins = http_plugins
for k, v in pairs(stream_plugins) do
allPlugins[k] = v
end
return 200, allPlugins
--
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] Revolyssup commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1575644885
@monkeyDluffy6017 Please approve CI
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1243428335
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,14 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- When name is unspecified, return all plugins for the given subsystem
Review Comment:
We have `/apisix/admin/plugins/list`, so we don't need this interface `/apisix/admin/plugins`
--
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] moonming commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1608806253
@monkeyDluffy6017 ping
--
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] monkeyDluffy6017 commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1608845966
@Revolyssup have you updated the pr based on the proposal?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263569720
##########
docs/zh/latest/admin-api.md:
##########
@@ -1319,6 +1319,14 @@ Content-Type: text/plain
Plugin 资源请求地址：/apisix/admin/plugins/{plugin_name}
+### 请求参数
+
+| 名称 | 描述 | 默认 |
+| --------- | -------------------------------------- | -------- |
+| subsystem | 插件子系统。 | http |
+
+可以在子系统上过滤插件，以便在通过查询参数传递的子系统中搜索 ({plugin_name})。请注意，此查询参数在“/apisix/admin/plugins/list”上不可用
Review Comment:
I think it's supported on /apisix/admin/plugins/list?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263560538
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ end
Review Comment:
```suggestion
if subsystem == "http" then
http_plugins = core.config.local_conf().plugins
else
stream_plugins = core.config.local_conf().stream_plugins
end
```
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263559497
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
Review Comment:
Could you check the `subsystem` here?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266156980
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,20 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
- end
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
+ else
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
- core.log.warn("failed to load plugin [", name, "] err: ", plugin)
- return 400, {error_msg = "failed to load plugin " .. name}
+ if not plugin then
+ local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
+ core.log.warn("failed to load plugin [", name, "] err: ", err)
+ return 404, {error_msg = "plugin not found"}
Review Comment:
"plugin " .. name .. " is not found in subsystem " .. subsystem
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266216955
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +99,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "http" then
+ http_plugins = core.config.local_conf().plugins
+ else
+ stream_plugins = core.config.local_conf().stream_plugins
+ end
+
+
Review Comment:
done
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264704135
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
Review Comment:
`subsystem` is being checked here - https://github.com/apache/apisix/pull/9580/files#diff-00090cde519e7280adbc73e830b92089c4e62a22057c1b7577c77dc1456e72e5R108
I didn't get you.
--
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] Revolyssup commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1612484993
@monkeyDluffy6017 I committed the last suggestions you had. Can you approve CI once?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254167305
##########
apisix/admin/init.lua:
##########
@@ -253,8 +254,9 @@ end
local function get_plugins_list()
set_ctx_and_check_token()
-
- local plugins = resources.plugins.get_plugins_list()
+ local args = get_uri_args()
+ local subsystem = args["subsystem"]
+ local plugins = resources.plugins.get_plugins_list(subsystem)
Review Comment:
`htt&stream` could not be accepted, it's not useless, so the value could be `http`, `stream`, `nil`
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254238947
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,15 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- arg all to be deprecated
+ if (arg and arg["all"] == "true") or not name then
Review Comment:
Applied the suggestion
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254234074
##########
apisix/admin/plugins.lua:
##########
@@ -85,8 +106,14 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local plugins
+ if subsystem == "stream" then
+ plugins = core.config.local_conf().stream_plugins
+ else
+ plugins = core.config.local_conf().plugins
+ end
+
Review Comment:
done
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1254152567
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,15 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- arg all to be deprecated
+ if (arg and arg["all"] == "true") or not name then
Review Comment:
```suggestion
if (arg and arg["all"] == "true") then
```
There is no need to change the original condition
--
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] Sn0rt commented on pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Sn0rt (via GitHub)" <gi...@apache.org>.
Sn0rt commented on PR #9580:
URL: https://github.com/apache/apisix/pull/9580#issuecomment-1622920631
@monkeyDluffy6017 pls help me to 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263558693
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
Review Comment:
Could you check the `subsystem` here?
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
+ local stream_plugins
+ if subsystem == "stream" then
Review Comment:
Could you check the `subsystem` here?
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264701437
##########
docs/en/latest/admin-api.md:
##########
@@ -1318,6 +1318,14 @@ Plugin resource request address: /apisix/admin/plugins/{plugin_name}
The Plugin ({plugin_name}) of the data structure.
+### Request Arguments
+
+| Name | Description | Default |
+| --------- | ----------------------------- | ------- |
+| subsystem | The subsystem of the Plugins. | http |
+
+The plugin can be filtered on subsystem so that the ({plugin_name}) is searched in the subsystem passed through query params. Note that this query parameter is not available on `/apisix/admin/plugins/list`
Review Comment:
It wasn't present present previously and was later added in the pull request. I removed the later comment.
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1263555124
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,23 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
+
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
end
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "stream" then
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
+ if not plugin then
+ subsystem = subsystem or "http"
+ local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
core.log.warn("failed to load plugin [", name, "] err: ", plugin)
- return 400, {error_msg = "failed to load plugin " .. name}
+ return 400, {error_msg = err }
Review Comment:
There is no need to return an error here? If the plugin doesn't exist, it is fine to return `nil`?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1266106958
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,15 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
Review Comment:
```suggestion
local subsystem = arg["subsystem"] or "http"
if subsystem ~= "http" and subsystem ~= "stream" then
return 400, {error_msg = "unsupported subsystem: "..subsystem}
end
```
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1213881550
##########
apisix/admin/plugins.lua:
##########
@@ -25,7 +25,6 @@ local get_uri_args = ngx.req.get_uri_args
local plugin_get_all = require("apisix.plugin").get_all
local encrypt_conf = require("apisix.plugin").encrypt_conf
local pairs = pairs
-
Review Comment:
Done @monkeyDluffy6017
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1243335900
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,14 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- When name is unspecified, return all plugins for the given subsystem
Review Comment:
So the output of /apisix/admin/plugins?subsystem=xyz would be all plugins of subsystem xyz
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1243430652
##########
apisix/admin/plugins.lua:
##########
@@ -42,7 +44,14 @@ end
function _M.get(name)
local arg = get_uri_args()
- if arg and arg["all"] == "true" then
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem ~= "http" and subsystem ~= "stream" then
+ return 400, {error_msg = "unsupported subsystem: "..subsystem}
+ end
+
+ -- When name is unspecified, return all plugins for the given subsystem
Review Comment:
`/apisix/admin/plugins` will return what `/apisix/admin/plugins?all=true` returns now. With full schema.
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219595831
##########
apisix/admin/plugins.lua:
##########
@@ -42,6 +42,11 @@ end
function _M.get(name)
local arg = get_uri_args()
+ -- If subsystem is passed inside args then it should be oneOf: http / stream.
+ local subsystem = arg["subsystem"]
+ if subsystem and subsystem~="" and subsystem~="http" and subsystem ~= "stream" then
Review Comment:
Why do you check `subsystem~=""`?
--
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] monkeyDluffy6017 commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1219627771
##########
apisix/admin/plugins.lua:
##########
@@ -53,11 +58,20 @@ function _M.get(name)
scope = true,
})
- if arg["subsystem"] == "stream" then
+ if subsystem == "stream" then
return 200, stream_plugins
end
-
- return 200, http_plugins
+ if subsystem == "http" then
Review Comment:
Add a blank
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264704476
##########
apisix/admin/plugins.lua:
##########
@@ -60,16 +70,23 @@ function _M.get(name)
return 200, http_plugins
end
- if not name then
- return 400, {error_msg = "not found plugin name"}
+ local plugin
+ -- By default search through http subsystems
+ subsystem = subsystem or "http"
+
+ if subsystem == "http" then
+ plugin = plugin_get_http(name)
end
- local plugin_name = "apisix.plugins." .. name
+ if subsystem == "stream" then
+ plugin = plugin_get_stream(name)
+ end
- local ok, plugin = pcall(require, plugin_name)
- if not ok then
+ if not plugin then
+ subsystem = subsystem or "http"
+ local err = "failed to load plugin " .. name .. " in subsystem " .. subsystem
core.log.warn("failed to load plugin [", name, "] err: ", plugin)
- return 400, {error_msg = "failed to load plugin " .. name}
+ return 400, {error_msg = err }
Review Comment:
done
--
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] Revolyssup commented on a diff in pull request #9580: fix: Improve results from admin API while GET all plugins
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9580:
URL: https://github.com/apache/apisix/pull/9580#discussion_r1264704265
##########
apisix/admin/plugins.lua:
##########
@@ -85,16 +102,37 @@ function _M.get(name)
end
-function _M.get_plugins_list()
- local plugins = core.config.local_conf().plugins
+function _M.get_plugins_list(subsystem)
+ local http_plugins
Review Comment:
You mean before defining `stream_plugins` variable?
--
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