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 default values & missing fields to Upstream #1764

Merged
merged 62 commits into from Apr 15, 2021

Conversation

juzhiyuan
Copy link
Member

@juzhiyuan juzhiyuan commented Apr 13, 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?

Hi, this PR aims to add the missing fields to Upstream Form, according to schema_def. (https://github.com/apache/apisix/blob/master/apisix/schema_def.lua#L114) And all default value are added.

  • Concurrency
  • ...

fix #1770

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

@netlify
Copy link

netlify bot commented Apr 13, 2021

Deploy preview for apisix-dashboard ready!

Built with commit 1890dc7

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

@juzhiyuan juzhiyuan marked this pull request as draft April 13, 2021 08:19
@codecov-io
Copy link

codecov-io commented Apr 13, 2021

Codecov Report

Merging #1764 (900e6a3) into master (537b4ff) will decrease coverage by 9.23%.
The diff coverage is n/a.

❗ Current head 900e6a3 differs from pull request most recent head 881f365. Consider uploading reports for the commit 881f365 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1764      +/-   ##
==========================================
- Coverage   71.55%   62.32%   -9.24%     
==========================================
  Files          47       47              
  Lines        3129     3129              
==========================================
- Hits         2239     1950     -289     
- Misses        646      866     +220     
- Partials      244      313      +69     
Flag Coverage Δ
backend-e2e-test 62.32% <ø> (ø)
backend-e2e-test-ginkgo 49.18% <ø> (-0.16%) ⬇️
backend-unit-test ?

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

Impacted Files Coverage Δ
api/internal/utils/runtime/runtime.go 0.00% <0.00%> (-64.29%) ⬇️
api/internal/core/store/validate_mock.go 0.00% <0.00%> (-50.00%) ⬇️
api/internal/filter/authentication.go 47.22% <0.00%> (-30.56%) ⬇️
api/internal/handler/service/service.go 62.60% <0.00%> (-29.57%) ⬇️
api/internal/core/store/store.go 58.68% <0.00%> (-29.35%) ⬇️
api/internal/filter/ip_filter.go 48.71% <0.00%> (-23.08%) ⬇️
api/internal/handler/global_rule/global_rule.go 64.51% <0.00%> (-19.36%) ⬇️
...pi/internal/handler/plugin_config/plugin_config.go 59.57% <0.00%> (-18.09%) ⬇️
api/internal/utils/json_patch.go 44.82% <0.00%> (-13.80%) ⬇️
api/internal/handler/upstream/upstream.go 77.14% <0.00%> (-13.58%) ⬇️
... and 12 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 537b4ff...881f365. Read the comment docs.

@juzhiyuan
Copy link
Member Author

CI passes on local

@moonming
Copy link
Member

@juzhiyuan please use a better title of this PR

@juzhiyuan juzhiyuan changed the title chore: added Fields to Upstream module feat: added default values & missing fields to Upstream Apr 15, 2021
required
rules={[{ required: true, message: "" }, { max: 64 * 1024 }, { min: 128 }]}
>
<Input.TextArea disabled={readonly} minLength={128} maxLength={64 * 1024} rows={5} placeholder="请输入客户端证书" />
Copy link
Member

Choose a reason for hiding this comment

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

@juzhiyuan The text of placeholder seems no i18n here. As expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, same as your last comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Due to CI will take a long time to run, I would prefer to remove them in the new PR.

required
rules={[{ required: true, message: "" }, { max: 64 * 1024 }, { min: 128 }]}
>
<Input.TextArea disabled={readonly} minLength={128} maxLength={64 * 1024} rows={5} placeholder="请输入客户端私钥" />
Copy link
Member

Choose a reason for hiding this comment

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

ditto here.

"deleteAlert": ".ant-modal-body"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

new line please.

if (targetData) {
form.setFieldsValue(transformUpstreamDataFromRequest(targetData));
} else {
// TODO: 提示 upstream_id 找不到想要的数据
Copy link
Member

Choose a reason for hiding this comment

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

remove Chinese comment.

required
rules={[{ required: true, message: "" }, { max: 64 * 1024 }, { min: 128 }]}
>
<Input.TextArea disabled={readonly} minLength={128} maxLength={64 * 1024} rows={5} placeholder="请输入客户端证书" />
Copy link
Member

Choose a reason for hiding this comment

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

ditto

required
rules={[{ required: true, message: "" }, { max: 64 * 1024 }, { min: 128 }]}
>
<Input.TextArea disabled={readonly} minLength={128} maxLength={64 * 1024} rows={5} placeholder="请输入客户端私钥" />
Copy link
Member

Choose a reason for hiding this comment

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

ditto

</Form.Item>
)
}

export default PassiveCheckTypeComponent
export default ActiveCheckTypeComponent
Copy link
Member

Choose a reason for hiding this comment

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

wrong name.

@juzhiyuan
Copy link
Member Author

juzhiyuan commented Apr 15, 2021

Hi @imjoey @LiteSun , thanks for your reviewing, just noticed there have some comments related to i18n (I missed them), if no urgent bug needs to be fixed, I would prefer resolving them in the new PR, because GitHub Action will queue unexpected those days.

Copy link
Member

@imjoey imjoey left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks @juzhiyuan .

@moonming moonming merged commit 4b86ef2 into apache:master Apr 15, 2021
@juzhiyuan juzhiyuan deleted the juzhiyuan/upstream-fields branch April 15, 2021 04:33
@juzhiyuan juzhiyuan mentioned this pull request Apr 15, 2021
3 tasks
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.

there is no default status code when configuring the health check through the Dashboard
6 participants