feature(http-client): add basic http-client implementation #216
Conversation
matheus-consoli
force-pushed
the
http-client
branch
2 times, most recently
from
f9ef201
to
e988671
Compare
matheus-consoli
force-pushed
the
http-client
branch
2 times, most recently
from
a5788b7
to
5afaf60
Compare
matheus-consoli
force-pushed
the
http-client
branch
from
5afaf60
to
b98cd9f
Compare
src/http_client/client.rs
Outdated
| ClientBuilder::default().build() | ||
| } | ||
|
|
||
| pub fn get(&self, uri: impl AsRef<str>) -> RequestBuilder { |
There was a problem hiding this comment.
is there reason for another layout of abstraction? Maybe this shouldn't be called Client, Something like ReadOnlyClient or specific purpose
There was a problem hiding this comment.
I removed the ClientBuilder layer and moved its contents to Client itself
tests/http_client.rs
Outdated
| #![cfg(all(any(unix, windows), feature = "http-client-json"))] | ||
|
|
||
| use fluvio_future::http_client::{self, ResponseExt, StatusCode}; | ||
|
|
There was a problem hiding this comment.
integrated test should not have outside dependency which could be offline or behavior changed. Please take look at other service testing. This means crate fake http server so we can control exact behavior to test
There was a problem hiding this comment.
There is a nice reference unit test in the hub-pkg-server repo https://github.com/infinyon/hub-service/blob/4841b25d4a13c6f2f035d2310c2600c3d1341b8c/crates/hub-pkg-server/src/util.rs#L314
There was a problem hiding this comment.
Setting up a standalone https server with a root-certificate derived cert is time consuming, I suggest we mark ignore on external sites referenced in this test, and keep the https://infinyon domain sites in for this test.
There was a problem hiding this comment.
this supposed to be have test for previous PR. There is already sample client/server test so it should be just matter of copy/paste. Integration test should not call external service since it should be able to execute locally. If there is test to rely on external root cert then it can marked as ignore and invoke separately by CI only.
matheus-consoli
force-pushed
the
http-client
branch
from
8a8e2c2
to
34e1d88
Compare
src/http_client/client.rs
Outdated
| } | ||
|
|
||
| pub fn get(&self, uri: impl AsRef<str>) -> RequestBuilder { | ||
| let req = http::request::Builder::new().uri(Uri::from_str(uri.as_ref()).unwrap()); |
|
|
||
| use super::client::Client; | ||
|
|
||
| pub struct RequestBuilder { |
There was a problem hiding this comment.
use https://crates.io/crates/derive_builder, it's been used in the fluvio successfully
|
|
||
| pub async fn send(self) -> Result<Response<Body>, Error> { | ||
| let req = self | ||
| .req_builder |
There was a problem hiding this comment.
let's not set default here. Put all defaults in the builder so it's configurable. using derive_builder will simplify effort
matheus-consoli
force-pushed
the
http-client
branch
from
e447fdc
to
2673975
Compare
|
Continued on #219 |
The first implementation of a mini HTTP client based on
reqwest's APIThe client uses hyper, rustls, and async-std.
The client supports bytes and
json(as an opt-in feature).Depends on #215