Skip to content

Conversation

markdingram
Copy link
Contributor

@markdingram markdingram commented Sep 24, 2024

fixes #1569

Motivation

Solution

hyper-util 0.1.9 unblocked this change by dropping the tower 0.4 dep -> hyperium/hyper-util#151

tower 0.5 includes this change to make Buffer generic over the Future rather than the Service -> tower-rs/tower#654

tower 0.5 includes this change to rework Either to no longer change the error type to BoxError, meaning Kube-RS now has to do so itself -> tower-rs/tower#637

tower-http 0.6.1 includes this change which promises to improve (de)compression -> tower-rs/tower-http#521

@markdingram markdingram force-pushed the tower-upgrade branch 2 times, most recently from f884bc6 to e97b01e Compare September 24, 2024 21:11
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 75.3%. Comparing base (acd2d8e) to head (6dd1938).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kube-client/src/client/config_ext.rs 0.0% 3 Missing ⚠️
kube-client/src/client/builder.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1589     +/-   ##
=======================================
- Coverage   75.3%   75.3%   -0.0%     
=======================================
  Files         82      82             
  Lines       7327    7328      +1     
=======================================
  Hits        5515    5515             
- Misses      1812    1813      +1     
Files with missing lines Coverage Δ
kube-client/src/client/mod.rs 75.2% <ø> (ø)
kube-client/src/client/builder.rs 58.6% <0.0%> (-0.7%) ⬇️
kube-client/src/client/config_ext.rs 50.0% <0.0%> (ø)

fixes kube-rs#1569

Signed-off-by: Mark Ingram <mark@lincs.dev>
@clux clux added dependencies upgrades to dependencies changelog-change changelog change category for prs labels Sep 24, 2024
@clux clux added this to the 0.96.0 milestone Sep 24, 2024
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this. From a quick pass this looks sensible to both me and CI.

// - `BoxService` for dynamic response future type
inner: Buffer<BoxService<Request<Body>, Response<Body>, BoxError>, Request<Body>>,
// - `BoxFuture` for dynamic response future type
inner: Buffer<Request<Body>, BoxFuture<'static, Result<Response<Body>, BoxError>>>,
Copy link
Member

Choose a reason for hiding this comment

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

This does seem like a nicer signature split. Interesting that it has to be 'static though. Is that a problem you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BoxServicecan only be created by moving the underlying Service - https://docs.rs/tower/latest/tower/util/struct.BoxService.html#impl-BoxService%3CT,+U,+E%3E whereas a BoxFuture can be created from a ref, hence generic over lifetime.

The kube::Client design owns the Buffer and to make Buffer Clone the Future needs to be 'static - https://tower-rs.github.io/tower/tower/buffer/struct.Buffer.html#impl-Clone-for-Buffer%3CReq,+F%3E

So 'static here is both unavoidable & fine (IMO!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar in the Tonic upgrade PR - https://github.com/hyperium/tonic/pull/1892/files

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining!

@clux clux merged commit bb9a44e into kube-rs:main Sep 25, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs dependencies upgrades to dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade to tower 0.5.0
2 participants