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

Release 4 #99

Closed
8 tasks done
Tracked by #62
mettke opened this issue Jun 15, 2020 · 22 comments
Closed
8 tasks done
Tracked by #62

Release 4 #99

mettke opened this issue Jun 15, 2020 · 22 comments

Comments

@mettke
Copy link
Contributor

mettke commented Jun 15, 2020

This is the tracking Issue for Release 4

ToDo

@mettke
Copy link
Contributor Author

mettke commented Jun 15, 2020

Removing default features

Currently it is very difficult to remove the reqwest and thus a dependency on openssl when using openidconnect. The reason for this is, that oauth-rs has reqwest as default feature and openidconnect carries that default feature with it without the possibility of disabling it.

Personally I would like to see a concept which is more like tokio which does not add anything by default but allows the user to specify what he/she wants.

Remove future-1 support

future-1 did not receive any update for 9 month now. Question is whether we want to continue supporting users who use it.

Replace failure with concrete error type

As described in #90 failure got deprecated. I'm however not comfortable with having a crate like failure, thiserror or anyhow in a library crate. Those crates are, in my opinion, made for easy error handling in end user crates/binaries. We, however, should have a look whether it is possible to create a concrete error type which is flexible enough to handling unknown error.

@mettke
Copy link
Contributor Author

mettke commented Jun 16, 2020

@ramosbugs I would like to hear your feedback before starting to work on this (sorry for the noisy if you just did not have time to get it to it - just making sure that you know)

@ramosbugs
Copy link
Owner

Removing default features

Currently it is very difficult to remove the reqwest and thus a dependency on openssl when using openidconnect. The reason for this is, that oauth-rs has reqwest as default feature and openidconnect carries that default feature with it without the possibility of disabling it.

I'm confused about this. Why doesn't disabling the default features on openidconnect remove the dependency on reqwest-09 and keep it from being built? https://github.com/ramosbugs/openidconnect-rs/blob/82aa3c6cf6ab39934e2cc64d5648282176444c26/Cargo.toml#L18

Personally I would like to see a concept which is more like tokio which does not add anything by default but allows the user to specify what he/she wants.

I generally agree, but this crate isn't very useful out of the box without an HTTP client implementation, and the vast majority of users will probably want to use a simple, built-in HTTP client like the reqwest-based one this crate provides. As long as there's a straightforward way to disable the default, I think it's fine to continue building a client by default.


Remove future-1 support

future-1 did not receive any update for 9 month now. Question is whether we want to continue supporting users who use it.

Sounds good. Let's remove both futures 0.1 and reqwest 0.9 (see #100 (comment)).


Replace failure with concrete error type

As described in #90 failure got deprecated. I'm however not comfortable with having a crate like failure, thiserror or anyhow in a library crate. Those crates are, in my opinion, made for easy error handling in end user crates/binaries. We, however, should have a look whether it is possible to create a concrete error type which is flexible enough to handling unknown error.

The concrete change to this crate's public API that #90 is proposing is to replace the errors returned by this library with types that implement the std::error::Error trait instead of failure::Fail. The thiserror crate only provides a proc macro for helping to generate these custom error enums, replacing the functionality provided by the failure crate. This change removes the public API dependence on the failure crate and replaces it with something that only depends on std.

The anyhow crate is the one intended for end user crates/binaries, and #90 only uses it in example code and documentation (it's only a dev-dependency). So, I'm in favor of merging #90 and making a similar change to openidconnect.

@mettke
Copy link
Contributor Author

mettke commented Jun 17, 2020

I'm confused about this. Why doesn't disabling the default features on openidconnect remove the dependency on reqwest-09 and keep it from being built? https://github.com/ramosbugs/openidconnect-rs/blob/82aa3c6cf6ab39934e2cc64d5648282176444c26/Cargo.toml#L18

Let me try to explain. If I add the following deps:

openidconnect = "1.0.1"

It will be "expanded" to the following due to default-features

openidconnect = { version = "1.0.1", features = ["reqwest-09"] }
oauth2 = { version = "3.0", features = ["reqwest-09"] }

If I now switch to reqwest 10 we get the following:

# Entry
openidconnect = { version = "1.0.1", features = ["reqwest-010"] }
# Expanded
openidconnect = { version = "1.0.1", features = ["reqwest-010", "reqwest-09"] }
oauth2 = { version = "3.0", features = ["reqwest-010", "reqwest-09"] }

reqwest-09 is still in there as it is in the default features. Now lets disable default features:

openidconnect = { version = "1.0.1", features = ["reqwest-010"] , default-features = false }
# Expanded
openidconnect = { version = "1.0.1", features = ["reqwest-010"] }
oauth2 = { version = "3.0", features = ["reqwest-010", "reqwest-09"] }

While it does remove the reqwest9 from openidconnect it does not remove it from oauth2. Because it still has it activated in its default-features and one is unable to remove it. Feel free to create a small cargo project with the dependency lines as above. You will see a lock file similar to this:

[...]
[[package]]
name = "oauth2"
version = "3.0.0"
  [...]
dependencies = [
  [...]
 "reqwest",
  [...]
]
[...]
[[package]]
name = "openidconnect"
version = "1.0.1"
  [...]
dependencies = [
  [...]
 "oauth2",
  [...]
]
[...]
[[package]]
name = "reqwest"
version = "0.9.24"
[...]

So there are two ways to solve this. Switch oauth-rs to default-features = false in openidconnect-rs or disable all features by default. This is still relevant even if we remove reqwest9 because if someone wants to implement his/her own client, they probably want to be able to disable reqwest10 completly.

@ramosbugs
Copy link
Owner

ohh I see. yes, let's add disable-features = false to the oauth2 import in openidconnect

@mettke
Copy link
Contributor Author

mettke commented Jul 7, 2020

@ramosbugs I would be ready for a release. So if you have the time - feel free. I will continue my destruction afterwards in openidconnect-rs 😉.

@ramosbugs
Copy link
Owner

@mettke
Copy link
Contributor Author

mettke commented Jul 9, 2020

Small correction for the release notes:

only the async/await futures-03 feature flag is now supported

there is no more future-03 flag :D. It got removed and the future crate as well ;)

@ramosbugs
Copy link
Owner

oops thanks! updated :-)

@maxdymond
Copy link
Contributor

Just wondering - what's the expected timeline on this release? Is it blocked on #97?

@ramosbugs
Copy link
Owner

@maxdymond: I just went ahead and released https://crates.io/crates/oauth2/4.0.0-alpha.2 with the device code support. A 4.0 stable release isn't blocked on any particular issue, but it probably makes sense to let the new API bake a bit before committing to not having subsequent breaking changes for this release.

@jtgeibel
Copy link
Contributor

reqwest now supports tokio 1.0 on master, so it may make sense to wait for a release there before doing a 4.0 release.

ramosbugs pushed a commit that referenced this issue Jan 6, 2021
This drops the feature `reqwest-010` and replaces it with `reqwest-011` based on the new `reqwest 0.11` release. The version of tokio used in tests is also bumped to the stable 1.0 release.

cc #99
@ramosbugs
Copy link
Owner

ramosbugs commented Jan 6, 2021

I just cut 4.0.0-alpha.3 with the reqwest 0.11/tokio 1.0 change but with the reqwest-011 feature flag renamed to reqwest now that we only support one version.

@halvko
Copy link

halvko commented Mar 12, 2021

@mettke It seems like #117 have been merged, should the issue be updated?

@mettke
Copy link
Contributor Author

mettke commented Mar 15, 2021

@halvko sure thing. I will close it as 4.0 is released by now and @ramosbugs will decide when it will leave alpha 👍🏻

@mettke mettke closed this as completed Mar 15, 2021
@Emilgardis
Copy link

4.0 is not released, its in alpha. I dont think this should be closed

@ximon18
Copy link
Contributor

ximon18 commented Mar 17, 2021

On the topic of a release, is there a plan for when a final release will be made?

@ramosbugs ramosbugs reopened this Mar 19, 2021
@ramosbugs
Copy link
Owner

I just released https://crates.io/crates/oauth2/4.0.0-beta.1, which should preclude further breaking changes. This bump did include a couple of small breaking changes, though.

I'll plan to let this version soak for about a month before cutting 4.0.0.

@ximon18
Copy link
Contributor

ximon18 commented Apr 12, 2021

Hi @ramosbugs. How's the release planning thinking going? I ask because we're nearing release of our product that uses these (oauth2 and openidconnect) beta versions and it would be great if we could switch to final versions for our release. Mainly just a "looks better" thing though as of course the functionality works fine as it is now.

@ramosbugs
Copy link
Owner

hey @ximon18, I'm planning to cut 4.0.0 in another week or so. I can't do any dev work for the next few days until my laptop gets back from being repaired.

@ximon18
Copy link
Contributor

ximon18 commented Apr 12, 2021

Thanks @ramosbugs. Good luck with the laptop!

@ramosbugs
Copy link
Owner

4.0.0 is now released!
https://github.com/ramosbugs/oauth2-rs/releases/tag/4.0.0
https://crates.io/crates/oauth2/4.0.0

Thanks to everyone who contributed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants