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.

Sign up for GitHub

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

Jump to bottom

fix: take the current path of the http request and put it into the router request #699 #705

Merged
merged 1 commit into from
Mar 21, 2022
Merged

fix: take the current path of the http request and put it into the router request #699 #705

merged 1 commit into from
Mar 21, 2022

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Mar 21, 2022

close #699

@bnjjj bnjjj added the bug label Mar 21, 2022
@bnjjj bnjjj requested review from BrynCooke and o0Ignition0o March 21, 2022 09:55
@bnjjj bnjjj self-assigned this Mar 21, 2022
@bnjjj bnjjj changed the title fix: take the current path of the request and put it into the router request #699 fix: take the current path of the http request and put it into the router request #699 Mar 21, 2022
@github-actions

This comment has been minimized.

Sign in to view
o0Ignition0o
o0Ignition0o approved these changes Mar 21, 2022
View reviewed changes
Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

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

LGTM, I'd love us to add an integration test (if possible) to make sure it doesn't change eventually

@bnjjj
fix: take the current path of the request and put it into the router …
dcf9d3d 
…request #699

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
o0Ignition0o
o0Ignition0o approved these changes Mar 21, 2022
View reviewed changes
apollo-router/src/warp_http_server_factory.rs
expectations
.expect_service_call()
.times(1)
.withf(|req| req.url().path() == "/graphql")
Copy link
Contributor

Choose a reason for hiding this comment

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

perfect thanks a lot! <3

@BrynCooke
Copy link
Contributor

Perf test please

@bnjjj bnjjj merged commit d44c534 into main Mar 21, 2022
@bnjjj bnjjj deleted the bnjjj/fix_699 branch March 21, 2022 10:52
@abernix abernix added this to the v0.1.0-alpha.10 milestone Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@o0Ignition0o o0Ignition0o o0Ignition0o approved these changes

@BrynCooke BrynCooke BrynCooke approved these changes

Assignees

@bnjjj bnjjj

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Plugin API, HTTP path is always /
4 participants
@bnjjj @BrynCooke @o0Ignition0o @abernix