Skip to content

feat: add plugin config#3567

Merged
membphis merged 4 commits into
apache:masterfrom
spacewander:plugin_config
Feb 20, 2021
Merged

feat: add plugin config#3567
membphis merged 4 commits into
apache:masterfrom
spacewander:plugin_config

Conversation

@spacewander

Copy link
Copy Markdown
Member

Fix #3532
Signed-off-by: spacewander [email protected]

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@spacewander
spacewander force-pushed the plugin_config branch 2 times, most recently from f94bdd7 to b3a6f79 Compare February 10, 2021 09:55
fix apache#3532
Signed-off-by: spacewander <[email protected]>
@spacewander
spacewander marked this pull request as ready for review February 10, 2021 10:30

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread apisix/cli/etcd.lua
"/plugins", "/consumers", "/node_status",
"/ssl", "/global_rules", "/stream_routes",
"/proto", "/plugin_metadata"}) do
"/proto", "/plugin_metadata", "/plugin_configs"}) do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to move this logic to the init_by_lua phase in the future?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After we can sync etcd data in that phase.

Comment thread apisix/init.lua Outdated
local enable_websocket = route.value.enable_websocket

if route.value.plugin_config_id then
local pc = plugin_config.get(route.value.plugin_config_id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pc is not a good name

Comment thread apisix/plugin_config.lua Outdated


function _M.merge(route_conf, plugin_config)
if route_conf.prev_plugin_config_ver ~= plugin_config.modifiedIndex then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

another code style:

if route_conf.prev_plugin_config_ver == plugin_config.modifiedIndex then
    return route_conf
end

...

Comment thread apisix/schema_def.lua
type = "object",
properties = {
id = id_schema,
desc = {type = "string", maxLength = 256},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

all of the desc should be the same, declare it as a variable and just quote it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Will do it in the next PR.

Signed-off-by: spacewander <[email protected]>
Comment thread apisix/admin/plugin_config.lua Outdated
local key = "/plugin_configs/" .. id
local res, err = core.etcd.delete(key)
if not res then
core.log.error("failed to delete global rule[", key, "]: ", err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: should be "failed to delete plugin_config"

Comment thread doc/architecture-design.md Outdated
Comment thread t/admin/plugin-configs.t
Signed-off-by: spacewander <[email protected]>
@membphis
membphis merged commit 013db0d into apache:master Feb 20, 2021
@membphis

Copy link
Copy Markdown
Member

merged, many thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

request help: what's the plan of plugin reference

3 participants