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: select Use the domain or IP from Node List #2168

Merged
merged 2 commits into from
Oct 20, 2021

Conversation

okaybase
Copy link
Member

@okaybase okaybase commented Oct 11, 2021

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?

When create Upstream and select Use the domain or IP from Node List, v2.8/v2.9.0 tip this and unable to select:
image

Related issues

fix/resolve:
PR(2118) change name="nodes" to name="submitNodes", so this PR sync the change in the file PassHost.tsx
image

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

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #2168 (33134e2) into master (a8298f4) will decrease coverage by 1.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2168      +/-   ##
==========================================
- Coverage   69.34%   68.24%   -1.11%     
==========================================
  Files         188      127      -61     
  Lines        7161     3347    -3814     
  Branches      823      823              
==========================================
- Hits         4966     2284    -2682     
+ Misses       1906     1063     -843     
+ Partials      289        0     -289     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test ?
frontend-e2e-test 68.24% <100.00%> (+0.08%) ⬆️

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

Impacted Files Coverage Δ
...eb/src/components/Upstream/components/PassHost.tsx 81.25% <100.00%> (+6.25%) ⬆️
api/internal/handler/label/label.go
api/internal/handler/schema/plugin.go
api/internal/handler/global_rule/global_rule.go
...pi/internal/handler/plugin_config/plugin_config.go
api/internal/filter/recover.go
api/internal/utils/consts/api_error.go
api/internal/filter/schema.go
api/internal/handler/data_loader/route_export.go
api/cmd/root.go
... and 53 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 a8298f4...33134e2. Read the comment docs.

@bzp2010
Copy link
Contributor

bzp2010 commented Oct 13, 2021

Hi, @okaybase. Thank you for your contribution.

After testing, I found some problems.

  1. First I make this settings.
    image

  2. Then I delete the only node.
    image

  3. Despite some problems with the data, I can still submit and move on to the next step.
    image

  4. However, the data is still pass, which leads to data inconsistency.
    image

Please test it to confirm whether there is this problem. 🤔

@nic-chen nic-chen requested a review from LiteSun October 13, 2021 06:58
@bzp2010
Copy link
Contributor

bzp2010 commented Oct 13, 2021

@bzp2010 You are right, after testing, the case(delete the only node then submit) also exists problem, i will fix it together.

@okaybase Nice jobs!

BTW, I found a issue maybe about it. #2155 Can you take a look?

@okaybase
Copy link
Member Author

okaybase commented Oct 15, 2021

@bzp2010 You are right, after testing, the case(delete the only node then submit) also exists problem, i will fix it together.

@okaybase Nice jobs!

BTW, I found a issue maybe about it. #2155 Can you take a look?

IMO, The current logic is correct( #2155 this is not a bug) @bzp2010

@okaybase
Copy link
Member Author

@bzp2010 after confirm and testing again, maybe the current logic for your test case is reasonable.
see the doc description: https://github.com/apache/apisix/blob/master/docs/en/latest/admin-api.md#upstream

image

@juzhiyuan juzhiyuan self-requested a review October 17, 2021 14:26
@liuxiran liuxiran merged commit 0fc4be7 into apache:master Oct 20, 2021
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.

bug: apisix-dashboard unable to set upstream.pass_host as node when node more than 1
5 participants