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 proxy-mirror plugin form #1725

Merged
merged 29 commits into from
Apr 14, 2021
Merged

Conversation

LiteSun
Copy link
Member

@LiteSun LiteSun commented Apr 9, 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 proxy-mirror plugin form
image

Related issues

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 9, 2021

Codecov Report

Merging #1725 (02baecd) into master (c68e2d2) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 02baecd differs from pull request most recent head 2b13d81. Consider uploading reports for the commit 2b13d81 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1725      +/-   ##
==========================================
+ Coverage   72.01%   72.07%   +0.06%     
==========================================
  Files         160      161       +1     
  Lines        5820     5826       +6     
  Branches      651      652       +1     
==========================================
+ Hits         4191     4199       +8     
+ Misses       1385     1383       -2     
  Partials      244      244              
Flag Coverage Δ
backend-e2e-test 62.30% <ø> (ø)
backend-e2e-test-ginkgo 49.29% <ø> (+0.22%) ⬆️
backend-unit-test 52.29% <ø> (ø)
frontend-e2e-test 72.68% <100.00%> (+0.13%) ⬆️

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

Impacted Files Coverage Δ
web/src/components/Plugin/UI/plugin.tsx 69.23% <100.00%> (+5.59%) ⬆️
web/src/components/Plugin/UI/proxy-mirror.tsx 100.00% <100.00%> (ø)
web/src/components/Plugin/PluginDetail.tsx 59.54% <0.00%> (+1.52%) ⬆️

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 c68e2d2...2b13d81. Read the comment docs.

@membphis
Copy link
Member

membphis commented Apr 9, 2021

Can you provide a screenshot of the effect?

@LiteSun
Copy link
Member Author

LiteSun commented Apr 9, 2021

Can you provide a screenshot of the effect?

sure.

Copy link
Member

@juzhiyuan juzhiyuan left a comment

Choose a reason for hiding this comment

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

@juzhiyuan
Copy link
Member

Please file a new PR to update the Drawer:

  • i18n
  • Form Style

@LiteSun
Copy link
Member Author

LiteSun commented Apr 11, 2021

Please file a new PR to update the Drawer:

  • i18n
  • Form Style

updated.

Comment on lines 31 to 34
const data = {
correctHost: 'http://127.0.0.1',
wrongHost: '127.0.0.1:1999'
}
Copy link
Member

Choose a reason for hiding this comment

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

Please write directly in the test case, not this layer of data structure

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I think it's more intuitive to write it this way.

wrongHost: '127.0.0.1:1999'
}

it('creates consumer with proxy-mirror form', function () {
Copy link
Member

Choose a reason for hiding this comment

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

why we need consumer?

web/cypress/support/commands.js Outdated Show resolved Hide resolved
web/src/components/Plugin/UI/plugin.tsx Show resolved Hide resolved
@@ -21,4 +21,7 @@ export default {
'component.step.select.pluginTemplate.select.option': '手动配置',
'component.plugin.pluginTemplate.tip1': '1. 若路由已配置插件,则插件模板数据将与已配置的插件数据合并。',
'component.plugin.pluginTemplate.tip2': '2. 插件模板相同的插件会覆盖掉原有的插件。',

// proxxy-mirror
'component.pluginForm.proxy-mirror.host.tooltip': '指定镜像服务地址,例如:http://127.0.0.1:9797(地址中需要包含 schema :http或https,不能包含 URI 部分)',
Copy link
Member

Choose a reason for hiding this comment

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

You should add judgment on the front end

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Here is tip for user
  2. the front-end will do a schema check on the data entered by the user

Comment on lines +47 to +65
cy.contains(this.domSelector.pluginCard, 'key-auth').within(() => {
cy.contains('Enable').click({
force: true,
});
});
cy.focused(this.domSelector.drawer).should('exist');
cy.get(this.domSelector.disabledSwitcher).click();
// edit codemirror
cy.get(this.domSelector.codeMirror)
.first()
.then((editor) => {
editor[0].CodeMirror.setValue(
JSON.stringify({
key: 'test',
}),
);
cy.contains('button', 'Submit').click();
});

Copy link
Member

Choose a reason for hiding this comment

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

why we need key-auth plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because an authentication plugin is required when creating a Consumer.

@netlify
Copy link

netlify bot commented Apr 12, 2021

Deploy preview for apisix-dashboard ready!

Built with commit 43a6890

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

@LiteSun LiteSun requested a review from moonming April 13, 2021 00:22
Copy link
Member

@juzhiyuan juzhiyuan left a comment

Choose a reason for hiding this comment

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

image

As you could see, we only have one field host here, but with the same contents 3 times. It's ok to update them in a new PR, GitHub Action is too slow to run..

web/src/components/Plugin/UI/proxy-mirror.tsx Show resolved Hide resolved
web/src/components/Plugin/locales/en-US.ts Show resolved Hide resolved
web/src/components/Plugin/locales/zh-CN.ts Show resolved Hide resolved
<Form.Item
label="host"
name="host"
extra={formatMessage({ id: 'component.pluginForm.proxy-mirror.host.extra' })}
Copy link
Member

Choose a reason for hiding this comment

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

Because we have tooltip, so we don't need this extra.

@juzhiyuan juzhiyuan merged commit 14166d9 into apache:master Apr 14, 2021
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