search

inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
220 stars 38 forks source link

Update rustls version #118

Closed jannschu closed 7 months ago

jannschu commented 8 months ago

Closes #117.

I removed the dangerous_configuration feature from tokio-rustls because it is now available by default, see release notes: https://github.com/rustls/rustls/releases/tag/v%2F0.22.0.

jannschu commented 8 months ago

Linked the wrong feature removal, updated text above.

inejge commented 7 months ago

Thanks for the PR, but it has a glaring problem: it doesn't compile, since rustls rearranged and redefined some types and traits. Not for the first time, and I know what to look for, but if you're submitting a PR, it would be nice to have it tested beforehand. Otherwise, bumping a couple of version numbers is not much of a contribution.

Even when the refactoring is done, the nightly CI job fails, probably because of rust-lang/rust#118297, but that's on me and I'll fix it before pushing the changes. The refactoring is not trivial at all, as some structs' innards went private, and some gained lifetime parameters and made certain operations more complicated.

I appreciate the effort to contribute, and I'll merge the PR, but please note the point about testing, as it makes the overall workflow much smoother.

jannschu commented 7 months ago

Hi, thanks for your feedback. I checked whether it compiled back when creating the pull request and even removed the mentioned feature flag. I can only assume I used the wrong feature flag when checking, probably did cargo check --features rustls (which succeeds) instead of the correct feature flag tls-rustls.

I wouldn't dare to just naively bump a version number in Cargo.toml and I suggest you should not assume contributions, even if small in number of lines, are made without effort. In this case it was a mistake and I apologize.

I also always check CI and I am sure if I had seen errors back then I would have looked into it. I only got errors a couple days ago which I attributed to a change in CI and did not further investigate it at that point. If I had known about the errors, be assured, I would have looked into it.

I also do not recommended merging code that is not working.