Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: plugin_configs should store with etcd prefix #2226

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

MizuhaHimuraki
Copy link
Contributor

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?

Use etcd prefix in plugin_configs template

Related issues
plugin_configs miss match when the etcd prefix is not '/apisix'

2021/11/29 19:40:06 [error] 12464#12464: *21668 [lua] init.lua:400: http_access_phase(): failed to fetch plugin config by id: 383574139283178878, client: 30.49.4.130, server: _, request: "GET /hello HTTP/2.0", host: "xxx.com"
# etcdctl get --prefix /apisix
...
/apisix-3b5e0cc5/upstreams/383566764287460734
{...}
/apisix-3b5e0cc5/upstreams/383570406268732798
{...}
/apisix/plugin_configs/383574139283178878
{...}
...

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

@MizuhaHimuraki MizuhaHimuraki changed the title pluginconfig use etcd prefix plugin_configs should store with etcd prefix Nov 29, 2021
@MizuhaHimuraki MizuhaHimuraki changed the title plugin_configs should store with etcd prefix fix: plugin_configs should store with etcd prefix Nov 29, 2021
@bzp2010 bzp2010 added backend bug Something isn't working labels Nov 30, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2021

Codecov Report

Merging #2226 (ad710f8) into master (a555684) will increase coverage by 7.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2226      +/-   ##
==========================================
+ Coverage   62.43%   69.89%   +7.45%     
==========================================
  Files          57      184     +127     
  Lines        3905     7257    +3352     
  Branches        0      824     +824     
==========================================
+ Hits         2438     5072    +2634     
- Misses       1185     1891     +706     
- Partials      282      294      +12     
Flag Coverage Δ
backend-e2e-test 44.19% <100.00%> (-0.11%) ⬇️
backend-e2e-test-ginkgo 52.06% <100.00%> (?)
backend-unit-test 49.16% <0.00%> (+0.03%) ⬆️
frontend-e2e-test 68.13% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/internal/core/store/storehub.go 72.59% <100.00%> (+17.03%) ⬆️
web/src/constants.ts 100.00% <0.00%> (ø)
web/src/components/PluginFlow/PluginFlow.tsx 1.42% <0.00%> (ø)
web/src/components/Plugin/UI/limit-conn.tsx 92.30% <0.00%> (ø)
web/src/components/Plugin/UI/plugin.tsx 91.66% <0.00%> (ø)
...nents/Upstream/components/ServiceDiscoveryArgs.tsx 100.00% <0.00%> (ø)
web/src/components/Upstream/components/TLS.tsx 10.00% <0.00%> (ø)
...b/src/components/Plugin/UI/referer-restriction.tsx 90.90% <0.00%> (ø)
web/src/pages/Consumer/service.ts 92.30% <0.00%> (ø)
web/src/pages/Setting/Setting.tsx 94.59% <0.00%> (ø)
... and 142 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a555684...ad710f8. Read the comment docs.

@bzp2010
Copy link
Contributor

bzp2010 commented Nov 30, 2021

LGTM, just wait the CI passed.

@bzp2010 bzp2010 merged commit d7b7722 into apache:master Nov 30, 2021
@zaunist
Copy link
Contributor

zaunist commented Nov 30, 2021

LGTM, just wait the CI passed.

Hi, in this PR, the backend-e2e-ginkgo just run alarm test, is this normal ?

@juzhiyuan
Copy link
Member

LGTM, just wait the CI passed.

Hi, in this PR, the backend-e2e-ginkgo just run alarm test, is this normal ?

cc @bzp2010

@zaunist
Copy link
Contributor

zaunist commented Nov 30, 2021

LGTM, just wait the CI passed.

Hi, in this PR, the backend-e2e-ginkgo just run alarm test, is this normal ?

cc @bzp2010

😂,I checked again and there is no problem, I was looking at it wrong before, this test is running so fast I can't believe it

@MizuhaHimuraki MizuhaHimuraki deleted the fix_config_miss_issue branch November 30, 2021 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants