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: added cors plugin form #1733

Merged
merged 25 commits into from
Apr 15, 2021
Merged

feat: added cors plugin form #1733

merged 25 commits into from
Apr 15, 2021

Conversation

guoqqqi
Copy link
Member

@guoqqqi guoqqqi commented Apr 10, 2021

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?
added cors plugin form
image

  • 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 #1

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 Apr 10, 2021

Codecov Report

Merging #1733 (0541bf3) into master (8b6080d) will decrease coverage by 20.19%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1733       +/-   ##
===========================================
- Coverage   72.45%   52.25%   -20.20%     
===========================================
  Files         132       38       -94     
  Lines        5721     2660     -3061     
  Branches      649        0      -649     
===========================================
- Hits         4145     1390     -2755     
+ Misses       1334     1082      -252     
+ Partials      242      188       -54     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test 52.25% <ø> (-0.04%) ⬇️
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%> (-74.77%) ⬇️
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 116 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 8b6080d...0541bf3. Read the comment docs.

@guoqqqi guoqqqi marked this pull request as draft April 10, 2021 03:38
@guoqqqi guoqqqi marked this pull request as ready for review April 10, 2021 09:59
@juzhiyuan
Copy link
Member

actually, the Form style needs to be updated. You could split it into 2 issues:

  • Features (Logics)
  • Styles & Friendly (Fields description/sample/...)

web/src/components/Plugin/UI/cors.tsx Outdated Show resolved Hide resolved
web/src/components/Plugin/UI/cors.tsx Outdated Show resolved Hide resolved
web/src/components/Plugin/UI/cors.tsx Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Apr 11, 2021

Deploy preview for apisix-dashboard ready!

Built with commit 4138c5b

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

@LiteSun LiteSun requested a review from liuxiran April 13, 2021 05:54
@LiteSun LiteSun requested a review from bzp2010 April 13, 2021 05:54
<Form
form={form}
{...FORM_ITEM_LAYOUT}
initialValues={{ allow_origins_by_regex: [''] }}
Copy link
Member

Choose a reason for hiding this comment

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

Why only add initial values for this field here?

'component.pluginForm.cors.allow_origins.extra': 'For example: https://somehost.com:8081',
'component.pluginForm.cors.allow_methods.tooltip': 'Which Method is allowed to enable CORS, such as: GET, POST etc. Multiple method use , to split. When allow_credential is false, you can use * to indicate allow all any method. You also can allow any method forcefully using ** even already enable allow_credential, but it will bring some security risks.',
'component.pluginForm.cors.allow_headers.tooltip': 'Which headers are allowed to set in request when access cross-origin resource. Multiple value use , to split. When allow_credential is false, you can use * to indicate allow all request headers. You also can allow any header forcefully using ** even already enable allow_credential, but it will bring some security risks.',
'component.pluginForm.cors.expose_headers.tooltip': ' Which headers are allowed to set in response when access cross-origin resource. Multiple value use , to split.',
Copy link
Member

Choose a reason for hiding this comment

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

Space??

<Select
mode="multiple"
optionLabelProp="label"
onChange={(value) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onChange={(value) => {
onChange={(value: string[]) => {

<Switch />
</Form.Item>

<Form.List name={['allow_origins_by_regex']}>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Form.List name={['allow_origins_by_regex']}>
<Form.List name="allow_origins_by_regex">

@LiteSun LiteSun merged commit fb75809 into apache:master Apr 15, 2021
@guoqqqi guoqqqi deleted the add-cors-plugin branch February 16, 2022 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants