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 scheme field test case #1661

Closed
wants to merge 2 commits into from

Conversation

batman-ezio
Copy link
Contributor

@batman-ezio batman-ezio commented Mar 24, 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?

add an optional field scheme on the upstream form

Related issues

fix/resolve #1642

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-io
Copy link

codecov-io commented Mar 24, 2021

Codecov Report

Merging #1661 (26afa8d) into master (1fa467a) will increase coverage by 0.44%.
The diff coverage is n/a.

❗ Current head 26afa8d differs from pull request most recent head ec569b1. Consider uploading reports for the commit ec569b1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1661      +/-   ##
==========================================
+ Coverage   71.35%   71.80%   +0.44%     
==========================================
  Files          47       47              
  Lines        3128     3128              
==========================================
+ Hits         2232     2246      +14     
+ Misses        656      639      -17     
- Partials      240      243       +3     
Flag Coverage Δ
backend-e2e-test 62.65% <ø> (+0.54%) ⬆️
backend-e2e-test-ginkgo 49.16% <ø> (+0.19%) ⬆️
backend-unit-test 52.29% <ø> (ø)

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

Impacted Files Coverage Δ
api/internal/utils/closer.go 33.33% <0.00%> (ø)
api/internal/core/store/store.go 87.95% <0.00%> (ø)
api/internal/core/store/storehub.go 74.76% <0.00%> (+3.73%) ⬆️
api/cmd/managerapi.go 59.45% <0.00%> (+5.40%) ⬆️
api/internal/core/storage/etcd.go 50.90% <0.00%> (+5.45%) ⬆️

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 1fa467a...ec569b1. Read the comment docs.

@juzhiyuan
Copy link
Member

cc @guoqqqi to take a look

@juzhiyuan juzhiyuan requested review from Jaycean and LiteSun March 25, 2021 07:57
@juzhiyuan
Copy link
Member

also cc @guoqqqi to review.

Copy link
Member

@guoqqqi guoqqqi left a comment

Choose a reason for hiding this comment

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

LGTM!

@membphis
Copy link
Member

membphis commented Apr 9, 2021

name="scheme"
>
<Select disabled={readonly}>
{['http', 'https', 'grpc', 'grpcs'].map((item, index) => (
Copy link
Member

Choose a reason for hiding this comment

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

I have never done .tsx before. I have done one other .tsx review. So similar my view is that an array or list should be ordered somehow and mostly its done alphabetically. I always like rules, standards and style guides.

Suggested change
{['http', 'https', 'grpc', 'grpcs'].map((item, index) => (
{['grpc', 'grpcs', 'http', 'https'].map((item, index) => (

@netlify
Copy link

netlify bot commented Apr 12, 2021

Deploy preview for apisix-dashboard ready!

Built with commit ec569b1

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

@batman-ezio batman-ezio changed the title feat: add scheme field to upstream form feat: add scheme field test case Apr 12, 2021
@juzhiyuan
Copy link
Member

I know what happened, one PR needs to be fixed.

@juzhiyuan
Copy link
Member

just wait for fixing it 😂

@nic-chen
Copy link
Member

@batman-ezio maybe you could sync code from master to try to fix FE CI

@github-actions
Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time ordiscuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 29, 2021
@github-actions
Copy link

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Nov 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whether to consider adding scheme configuration items in the upstream configuration
10 participants