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: make service chash key Input inputable and selectable #1982

Merged
merged 6 commits into from
Jul 30, 2021

Conversation

foolwc
Copy link
Contributor

@foolwc foolwc commented Jul 14, 2021

Fixes #1952

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?

Please update this section with detailed description.

Related issues

fix/resolve #1952

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

@netlify
Copy link

netlify bot commented Jul 14, 2021

✔️ Deploy Preview for apisix-dashboard ready!

🔨 Explore the source changes: 46fe51c

🔍 Inspect the deploy log: https://app.netlify.com/sites/apisix-dashboard/deploys/61020760be63c30007cd88fa

😎 Browse the preview: https://deploy-preview-1982--apisix-dashboard.netlify.app

@foolwc foolwc changed the title fix: make service chash key Input can inputable and selectable fix: make service chash key Input inputable and selectable Jul 14, 2021
@liuxiran
Copy link
Contributor

@foolwc thanks for your nice work~! The current modification is LGTM, it can support customize the hash key.

And If you can add some test cases, it will help us merge your PR more quickly.

Just refer to

it('should create chash upstream', function () {
, you can try to add case about when choose hash_on cookie, you can type key a customize one and submit successfully.

And refer to

it('should view the (chash) upstream', function () {
, you can try to add case about view the above upstream info as expected.

Thanks again, and looking forward to your test cases. If you have any question, please leave message here, we'll help you

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2021

Codecov Report

Merging #1982 (8d093bd) into master (1e55aa3) will increase coverage by 0.04%.
The diff coverage is 81.25%.

❗ Current head 8d093bd differs from pull request most recent head 46fe51c. Consider uploading reports for the commit 46fe51c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1982      +/-   ##
==========================================
+ Coverage   64.01%   64.05%   +0.04%     
==========================================
  Files         122      122              
  Lines        3154     3158       +4     
  Branches      753      754       +1     
==========================================
+ Hits         2019     2023       +4     
  Misses       1135     1135              
Flag Coverage Δ
frontend-e2e-test 64.05% <81.25%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
web/src/components/Plugin/service.ts 96.15% <ø> (ø)
web/src/components/Upstream/components/Nodes.tsx 100.00% <ø> (ø)
.../src/pages/Route/components/Step1/ProxyRewrite.tsx 94.44% <ø> (ø)
web/src/pages/Route/components/Step3/index.tsx 51.85% <0.00%> (ø)
web/src/pages/Consumer/Create.tsx 76.08% <50.00%> (ø)
web/src/components/Plugin/PluginDetail.tsx 65.60% <100.00%> (ø)
web/src/components/Upstream/components/Type.tsx 100.00% <100.00%> (ø)
web/src/global.tsx 36.58% <100.00%> (ø)
web/src/pages/Consumer/List.tsx 90.00% <100.00%> (ø)

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 1e55aa3...46fe51c. Read the comment docs.

@foolwc
Copy link
Contributor Author

foolwc commented Jul 26, 2021

@foolwc thanks for your nice work~! The current modification is LGTM, it can support customize the hash key.

And If you can add some test cases, it will help us merge your PR more quickly.

Just refer to

it('should create chash upstream', function () {

, you can try to add case about when choose hash_on cookie, you can type key a customize one and submit successfully.
And refer to

it('should view the (chash) upstream', function () {

, you can try to add case about view the above upstream info as expected.
Thanks again, and looking forward to your test cases. If you have any question, please leave message here, we'll help you

Thanks for your detailed explanation of writing e2e test. Please help me review my code again. Thanks a lot.

@juzhiyuan juzhiyuan requested a review from liuxiran July 26, 2021 06:25
@liuxiran
Copy link
Contributor

Thanks for your test cases, it is ok for the whole idea and process👏, and suggestions for improvement I have attached to the review.
To ensure test coverage, we also need test cases for Route and Upstream module, since actually you update the component. so looking forward to your other test cases @foolwc thanks again~!

@liuxiran
Copy link
Contributor

Nice work @foolwc , let's wait for CI
Also cc @guoqqqi @iamayushdas to take a look.

Thanks~

Co-authored-by: litesun <sunyi@apache.org>
@liuxiran liuxiran merged commit d49b283 into apache:master Jul 30, 2021
@liuxiran
Copy link
Contributor

merged, thanks for your contribution @foolwc

@foolwc foolwc deleted the chash-key-input branch July 30, 2021 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants