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

Update rustls version #118

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Update rustls version #118

merged 1 commit into from
Feb 14, 2024

Conversation

jannschu
Copy link

@jannschu jannschu commented Jan 19, 2024

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
Copy link
Author

Linked the wrong feature removal, updated text above.

@inejge
Copy link
Owner

inejge commented Feb 14, 2024

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.

@inejge inejge merged commit 3d0e091 into inejge:master Feb 14, 2024
1 of 13 checks passed
@jannschu
Copy link
Author

jannschu commented Feb 15, 2024

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.

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

Successfully merging this pull request may close these issues.

Bump rustls version to 0.22
2 participants