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: add the service page upstream select option #1633

Merged
merged 27 commits into from
May 3, 2021
Merged

feat: add the service page upstream select option #1633

merged 27 commits into from
May 3, 2021

Conversation

stu01509
Copy link
Contributor

@stu01509 stu01509 commented Mar 20, 2021

Please answer these questions before submitting a pull request


New feature or improvement

  • Describe the details and related test reports.
    Add the None option in the service page upstream select.

image

@codecov-io
Copy link

codecov-io commented Mar 20, 2021

Codecov Report

Merging #1633 (a0a72a5) into master (2c158a1) will decrease coverage by 19.79%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1633       +/-   ##
===========================================
- Coverage   72.08%   52.29%   -19.80%     
===========================================
  Files         130       38       -92     
  Lines        5649     2660     -2989     
  Branches      648        0      -648     
===========================================
- Hits         4072     1391     -2681     
+ Misses       1333     1081      -252     
+ Partials      244      188       -56     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test 52.29% <ø> (ø)
frontend-e2e-test ?

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

Impacted Files Coverage Δ
api/internal/utils/version.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/filter/request_id.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/core/entity/entity.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/core/store/storehub.go 0.00% <0.00%> (-71.03%) ⬇️
api/internal/filter/cors.go 0.00% <0.00%> (-66.67%) ⬇️
api/internal/filter/schema.go 0.00% <0.00%> (-55.47%) ⬇️
api/internal/utils/consts/api_error.go 0.00% <0.00%> (-50.00%) ⬇️
api/internal/handler/data_loader/route_import.go 27.41% <0.00%> (-37.50%) ⬇️
api/internal/handler/handler.go 42.59% <0.00%> (-35.19%) ⬇️
api/internal/handler/schema/schema.go 66.66% <0.00%> (-33.34%) ⬇️
... and 114 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 2c158a1...a0a72a5. Read the comment docs.

@juzhiyuan juzhiyuan requested a review from LiteSun March 20, 2021 15:06
@LiteSun
Copy link
Member

LiteSun commented Mar 21, 2021

cc @guoqqqi to check the e2e.

@juzhiyuan
Copy link
Member

ping @guoqqqi @LiteSun

@guoqqqi guoqqqi mentioned this pull request Mar 22, 2021
4 tasks
@juzhiyuan
Copy link
Member

may sync the master branch

@stu01509
Copy link
Contributor Author

may sync the master branch

Done 😀

@stu01509
Copy link
Contributor Author

stu01509 commented Mar 26, 2021

Hi @liuxiran

Thanks for your detailed explanation, I update the commit please review it again.

image

@guoqqqi
Copy link
Member

guoqqqi commented Mar 27, 2021

Hi, @stu01509 I noticed that when editing the service, the upstream information that was customized at creation disappeared, which I think should be visible here.
image

@stu01509
Copy link
Contributor Author

Hi @guoqqqi

Update the commit, I change the LINE#92

cy.contains('None').should('be.visible');

@guoqqqi
Copy link
Member

guoqqqi commented Mar 31, 2021

Hi @guoqqqi

Update the commit, I change the LINE#92

cy.contains('None').should('be.visible');

No, What I mean is that the data upstream of the creation of the service is now lost when editing.

@stu01509
Copy link
Contributor Author

stu01509 commented Apr 6, 2021

Hi @liuxiran

Thanks for your instruction, I add a condition in Service/Create.tsx LINE #67, the service can successful create with None upstream, but cypress still failed, according to my change, is it caused another error?

@liuxiran
Copy link
Contributor

liuxiran commented Apr 7, 2021

Hi @liuxiran

Thanks for your instruction, I add a condition in Service/Create.tsx LINE #67, the service can successful create with None upstream, but cypress still failed, according to my change, is it caused another error?

Hi @stu01509 , you have to deal with the confilct first, thanks

2021-04-07 12-35-57屏幕截图

Judging from the error log, when creating a service, cypress try to find dom #nodes_0_host, but never did it. That is because we enabled None option as the default selection, the #nodes_0_host dom will hidden.

so it would be better to set the default upstream selection to Custom.

@stu01509
Copy link
Contributor Author

stu01509 commented Apr 9, 2021

Hi @juzhiyuan @liuxiran @nic-chen, and @guoqqqi

I update the commit, and fix the ci error, please review it again, thanks.

@nic-chen
Copy link
Member

hi @stu01509
Conflicts, please sync code from branch master.

@juzhiyuan
Copy link
Member

@stu01509 Hi, I just noticed this PR, once this conflicts get resolved, I will continue reviewing it.

@stu01509
Copy link
Contributor Author

Ok, I will solve it later.

@netlify
Copy link

netlify bot commented Apr 13, 2021

Deploy preview for apisix-dashboard ready!

Built with commit cfe100c

https://deploy-preview-1633--apisix-dashboard.netlify.app

@liuxiran
Copy link
Contributor

Since #1784 has been merged, @stu01509 have to fix the conflicting files, then let's go on reviewing, thanks

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2021

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.13%. Comparing base (2f3717f) to head (cfe100c).
Report is 362 commits behind head on master.

Files with missing lines Patch % Lines
web/src/pages/Service/Create.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1633      +/-   ##
==========================================
+ Coverage   72.07%   72.13%   +0.05%     
==========================================
  Files         125      125              
  Lines        2944     2943       -1     
  Branches      711      710       -1     
==========================================
+ Hits         2122     2123       +1     
+ Misses        822      820       -2     
Flag Coverage Δ
frontend-e2e-test 72.13% <66.66%> (+0.05%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@liuxiran liuxiran left a comment

Choose a reason for hiding this comment

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

Hi @stu01509, after fix this problem, please continue to fix can not submit data in the last step of creating service when select upstream None

hope this commit can help you: liuxiran@d75b137

thanks

@juzhiyuan juzhiyuan merged commit e89665f into apache:master May 3, 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.

when creating a Service, Select Upstream should be optional, not required
9 participants