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

Fix broken nextUrl= parameter logic #940

Merged
merged 7 commits into from
Apr 19, 2022

Conversation

peternied
Copy link
Member

@peternied peternied commented Apr 11, 2022

Description

The nextUrl logic was getting back urls that looked like %2Fgotundefined it seems like with the recent node version change there were some breaking changes in the url parsing/processing systems. I've cleaned up the pattern and confirmed the behavior is correct with new integration tests.

Category

Bug fix

Issues Resolved

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@peternied peternied added bug Something isn't working v2.0.0 labels Apr 11, 2022
@peternied peternied marked this pull request as ready for review April 13, 2022 23:26
@peternied peternied requested a review from a team April 13, 2022 23:26
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2022

Codecov Report

Merging #940 (7e90a7b) into main (25263e7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #940   +/-   ##
=======================================
  Coverage   72.14%   72.14%           
=======================================
  Files          87       87           
  Lines        1906     1906           
  Branches      242      242           
=======================================
  Hits         1375     1375           
  Misses        477      477           
  Partials       54       54           

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 25263e7...7e90a7b. Read the comment docs.

@cliu123 cliu123 removed the v2.0.0 label Apr 15, 2022
@cliu123
Copy link
Member

cliu123 commented Apr 15, 2022

Removing the label for 2.0.0 as the PR hasn't been ready yet.

@kavilla
Copy link
Member

kavilla commented Apr 16, 2022

Nice! I have just created an issue for this: #951.

This is blocking tests from succeeding in 2.0 I think it should be prioritized or block 2.0.0 since I can imagine a lot of people getting this issue and the application would seem unusable.

@kavilla kavilla added the v2.0.0 label Apr 16, 2022
@cliu123 cliu123 force-pushed the fix-next-url branch 2 times, most recently from 168126d to cd79940 Compare April 18, 2022 06:20
@DarshitChanpura DarshitChanpura force-pushed the fix-next-url branch 2 times, most recently from 071fdce to 562213f Compare April 18, 2022 21:45
Signed-off-by: Darshit Chanpura <[email protected]>
@DarshitChanpura DarshitChanpura merged commit 611ef5d into opensearch-project:main Apr 19, 2022
@peternied peternied deleted the fix-next-url branch April 20, 2022 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove usage of legacy url API .query
5 participants