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

feat: support enable/disable route #2943

Merged
merged 8 commits into from
Dec 9, 2020

Conversation

tzssangglass
Copy link
Member

fix #2737

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

@tzssangglass tzssangglass changed the title feature: support enable/disable route feat: support enable/disable route Dec 3, 2020
@juzhiyuan juzhiyuan added the enhancement New feature or request label Dec 3, 2020
@liuxiran
Copy link
Contributor

liuxiran commented Dec 3, 2020

Thanks for you pr :), just wait for others preview.

@membphis
Copy link
Member

membphis commented Dec 3, 2020

nice PR

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

please fix other similar points

doc/zh-cn/admin-api.md Outdated Show resolved Hide resolved
t/lib/server.lua Outdated Show resolved Hide resolved
t/node/route-status.t Outdated Show resolved Hide resolved
t/node/route-status.t Outdated Show resolved Hide resolved
t/node/route-status.t Outdated Show resolved Hide resolved
t/node/route-status.t Outdated Show resolved Hide resolved
t/node/route-status.t Outdated Show resolved Hide resolved
@tzssangglass
Copy link
Member Author

solve above

@tzssangglass
Copy link
Member Author

tzssangglass commented Dec 3, 2020

describe a strange phenomenon, I was using /route_status api and create new routes because I found that using route id 1 and /hello always failed when I ran my local test cases, even through other test cases, like

$prove -I../test-nginx/lib -I./ -r -s t/router/radixtree-host-uri.t         
t/router/radixtree-host-uri.t .. 10/? 
#   Failed test 'TEST 5: hit routes - status code ok'
#   at /usr/local/apisix/../test-nginx/lib/Test/Nginx/Socket.pm line 948.
#          got: '404'
#     expected: '200'

#   Failed test 'TEST 5: hit routes - response_body - response is expected (repeated req 0, req 0)'
#   at /usr/local/apisix/../test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: '{"error_msg":"404 Route Not Found"}
# '
#     expected: 'hello world
# '

#   Failed test 'TEST 6: hit routes - status code ok'
#   at /usr/local/apisix/../test-nginx/lib/Test/Nginx/Socket.pm line 948.
#          got: '404'
#     expected: '200'

#   Failed test 'TEST 6: hit routes - response_body - response is expected (repeated req 0, req 0)'
#   at /usr/local/apisix/../test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: '{"error_msg":"404 Route Not Found"}
# '
#     expected: 'hello world
# '

until I debugged at

create_radixtree_router(user_routes.values)

when run test, the user_routes.values is nil, but when start apisix, it work.

after find this exception, something change, run the test case is back to normal, and cannot be reproduced. >_<|||

@liuxiran
Copy link
Contributor

liuxiran commented Dec 4, 2020

is it necessary to add a case:
create a route disabled -> hit route failed-> enable it -> hit route successfully?

t/node/route-status.t Outdated Show resolved Hide resolved
@liuxiran
Copy link
Contributor

liuxiran commented Dec 4, 2020

describe a strange phenomenon, I was using /route_status api and create new routes because I found that using route id 1 and /hello always failed when I ran my local test cases, even through other test cases, like

$prove -I../test-nginx/lib -I./ -r -s t/router/radixtree-host-uri.t         
t/router/radixtree-host-uri.t .. 10/? 
#   Failed test 'TEST 5: hit routes - status code ok'
#   at /usr/local/apisix/../test-nginx/lib/Test/Nginx/Socket.pm line 948.
#          got: '404'
#     expected: '200'

#   Failed test 'TEST 5: hit routes - response_body - response is expected (repeated req 0, req 0)'
#   at /usr/local/apisix/../test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: '{"error_msg":"404 Route Not Found"}
# '
#     expected: 'hello world
# '

#   Failed test 'TEST 6: hit routes - status code ok'
#   at /usr/local/apisix/../test-nginx/lib/Test/Nginx/Socket.pm line 948.
#          got: '404'
#     expected: '200'

#   Failed test 'TEST 6: hit routes - response_body - response is expected (repeated req 0, req 0)'
#   at /usr/local/apisix/../test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: '{"error_msg":"404 Route Not Found"}
# '
#     expected: 'hello world
# '

until I debugged at

create_radixtree_router(user_routes.values)

when run test, the user_routes.values is nil, but when start apisix, it work.

after find this exception, something change, run the test case is back to normal, and cannot be reproduced. >_<|||

If this magical event happened next, you may cc
@membphis @spacewander @tokers for help😊

@tzssangglass
Copy link
Member Author

is it necessary to add a case:
create a route disabled -> hit route failed-> enable it -> hit route successfully?

test case 1 ~ 4 is testing for uri match, the order is
create a route enable(default) -> hit route successfully-> disable it -> hit route fail

test case 5 ~ 8 is testing for host + uri match, the order is same as above.

I think combining these four steps into one test case would have the same effect.

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM, except minor style things

apisix/http/router/radixtree_host_uri.lua Outdated Show resolved Hide resolved
apisix/http/router/radixtree_uri.lua Outdated Show resolved Hide resolved
t/node/route-status.t Outdated Show resolved Hide resolved
t/node/route-status.t Outdated Show resolved Hide resolved
doc/admin-api.md Show resolved Hide resolved
doc/zh-cn/admin-api.md Show resolved Hide resolved
@moonming
Copy link
Member

moonming commented Dec 5, 2020 via email

t/node/route-status.t Outdated Show resolved Hide resolved
@spacewander spacewander merged commit c7b198f into apache:master Dec 9, 2020
@tzssangglass tzssangglass deleted the IssusNo2737 branch December 9, 2020 02:40
@tzssangglass
Copy link
Member Author

thanks to the people at code review, and thanks for the guidance!

spacewander added a commit to spacewander/incubator-apisix that referenced this pull request Dec 9, 2020
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
spacewander added a commit that referenced this pull request Dec 9, 2020
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Support enable/disable route
9 participants