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: Merge path hostname input box #2347

Closed
wants to merge 17 commits into from
Closed

feat: Merge path hostname input box #2347

wants to merge 17 commits into from

Conversation

Si-ege
Copy link
Contributor

@Si-ege Si-ege commented Feb 25, 2022

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?

Please update this section with detailed description.

Related issues

fix/resolve #733
1.Merge path hostname input box.
2.Splitting paths and hostnames before sending requests.

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

@zaunist
Copy link
Contributor

zaunist commented Feb 25, 2022

Hi, @Si-ege . Could you remove "()" from title ? I think it's a little strange.

@Si-ege Si-ege changed the title feat: ()Merge path hostname input box feat: Merge path hostname input box Feb 25, 2022
@guoqqqi
Copy link
Member

guoqqqi commented Feb 27, 2022

Hi @Si-ege I see that some of the changes were made by #2327, are you developing on the same branch, please make sure you have a clean branch for each development

@guoqqqi
Copy link
Member

guoqqqi commented Mar 7, 2022

ping @Si-ege

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2022

Codecov Report

Merging #2347 (506206e) into master (eb51353) will increase coverage by 6.55%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2347      +/-   ##
==========================================
+ Coverage   62.04%   68.60%   +6.55%     
==========================================
  Files          57      131      +74     
  Lines        3905     3437     -468     
  Branches        0      830     +830     
==========================================
- Hits         2423     2358      -65     
+ Misses       1197     1079     -118     
+ Partials      285        0     -285     
Flag Coverage Δ
backend-e2e-test ?
backend-unit-test ?
frontend-e2e-test 68.60% <100.00%> (?)

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

Impacted Files Coverage Δ
web/src/components/Upstream/UpstreamForm.tsx 90.12% <ø> (ø)
...ents/Upstream/components/active-check/HttpPath.tsx 100.00% <ø> (ø)
web/src/pages/Upstream/Create.tsx 94.54% <100.00%> (ø)
api/internal/filter/authentication.go
api/internal/filter/recover.go
api/internal/filter/cors.go
api/internal/core/storage/storage_mock.go
api/internal/conf/conf.go
api/internal/route.go
api/internal/core/migrate/dataset.go
... and 181 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 eb51353...506206e. Read the comment docs.

@guoqqqi
Copy link
Member

guoqqqi commented Mar 14, 2022

also CC @Baoyuantop PTAL

Comment on lines 50 to 52
const { active: activeData } = newData.checks;
delete activeData.host;
delete activeData.http_path;
Copy link
Contributor

Choose a reason for hiding this comment

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

We would prefer to use a deep clone before these operations.
I see that the operation in line 37 is not quite suitable, can you help me to change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, this is only a suggestion and not a necessity.

@@ -55,6 +55,7 @@ context('Create and Delete Upstream', () => {
weight0: '2',
port1: '7001',
weight1: '2',
url_input: 'www.baidu.com/test/asd',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to test for incorrect inputs?

Comment on lines 50 to 51
const url = host + http_path;
newData.checks.active.url = url;
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to use template literals.

Copy link
Member

Choose a reason for hiding this comment

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

means using this syntax
image

@guoqqqi
Copy link
Member

guoqqqi commented Mar 20, 2022

It seems that the front-end CI is unstable and we can commit an empty commit to trigger the CI
https://www.freecodecamp.org/news/how-to-push-an-empty-commit-with-git/

const url = `${host}${http_path}`;
const { active: activeData } = newData.checks;
activeData.url = url;
newData.checks.active = omit(newData.checks.active, 'host');
Copy link
Member

Choose a reason for hiding this comment

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

cc @hanzhenfang PTAL

@Si-ege Si-ege closed this by deleting the head repository Dec 30, 2023
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.

Operation experience optimization
5 participants