search

cduvray / jwt-authorizer

JWT authorization layer for Axum.
MIT License
69 stars 21 forks source link

Re-add support for tonic #54

Closed sjoerdsimons closed 2 months ago

sjoerdsimons commented 2 months ago

Now that tonic with hyper 1.0 is available re-add the support that was temporarily removed

Fixes #52

sjoerdsimons commented 2 months ago

The CI test fails as prost has a MSRV of 1.70; I guess to solve this we can either update the MSRV of this crate to match or only support leave it 1.68 as long as tonic is not supported

Fwiw 1.70 was release in june last year, while 1.68 was march last year

lcmgh commented 2 months ago

Just checked the MSRV policies of other players in the eco system.

Tokio will keep a rolling MSRV (minimum supported rust version) policy of at least 6 months. The current MSRV is 1.70

axum's MSRV is 1.66.

imho it is fair to upgrade this crates one to 1.70 (as over one year passed) but it would be higher than axum's.

sjoerdsimons commented 2 months ago

Just checked the MSRV policies of other players in the eco system.

Tokio will keep a rolling MSRV (minimum supported rust version) policy of at least 6 months. The current MSRV is 1.70

axum's MSRV is 1.66.

imho it is fair to upgrade this crates one to 1.70 (as over one year passed) but it would be higher than axum's.

Actually turns out the jsonwebtoken 9.3 already needs rust 1.73 regardless... So bumped to that now and only changed CI to only check the library build (msrv of the dev-dependency is typically not relevant)

lcmgh commented 2 months ago

kindly ping @cduvray for approval and release

cduvray commented 2 months ago

Hi, thanks for your PR. I reverted the commit 5f71827bf24a56f61f578e491c04176502fe3868, could you please rebase your PR on main, this will make the history more clear .

Ok for MSRV (I think I had errors even with 1.73, du to a dependency, the version 1.75 solves this), but in your commit I think no tests are run for the version MSRV is this what you want? I think tests should be run also for MSRV,

sjoerdsimons commented 2 months ago

Hi, thanks for your PR. I reverted the commit 5f71827, could you please rebase your PR on main, this will make the history more clear .

Done! Downside ofcourse that bisectability isn't as good but such is life :)

Ok for MSRV (I think I had errors even with 1.73, du to a dependency, the version 1.75 solves this), but in your commit I think no tests are run for the version MSRV is this what you want? I think tests should be run also for MSRV,

Yes the goal was to only build the library with MSRV to ensure it can be compiled with that rust version; The problem with also running tests on MSRV is that all the dev-dependencies then need a compatible MSRV as well, which imho is harder to maintain and less relevant for crate users.

lcmgh commented 2 months ago

The revert that landed on main put axum 6 back into the game: https://github.com/cduvray/jwt-authorizer/issues/56

cduvray commented 2 months ago

I've partially merged it (to have a usable main, and to solve #56), I'm not yet sure about CI and MSRV, I have to think about it.

sjoerdsimons commented 2 months ago

I've partially merged it (to have a usable main, and to solve #56), I'm not yet sure about CI and MSRV, I have to think about it.

Thanks! If you don't want to take these CI changes at the moment the easiest fix to get CI on main green again is likely to bump up msrv to 1.75 (not tested).

Fwiw this also shows the issue i was trying to fix as fundamentally now your MSRV is driven by your dev-dependencies instead (iirc through wiremock) rather then the actual library dependencies

cduvray commented 2 months ago

Hello, I've rewritten the CI bumping msrv to 75. I will close this PR as all has been cherry picked (except for CI).