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

chore(quic): deprecate QUIC draft-29 version support #5786

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tesol2y090
Copy link

@tesol2y090 tesol2y090 commented Dec 31, 2024

Description

Deprecate support_draft_version field from QUIC protocol.

resolves #3395

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@tesol2y090 tesol2y090 marked this pull request as ready for review December 31, 2024 14:04
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

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

LGTM. Left a small comment :)

transports/quic/CHANGELOG.md Outdated Show resolved Hide resolved
@dariusc93 dariusc93 changed the title feat(quic): deprecate QUIC draft-29 version support chore(quic): deprecate QUIC draft-29 version support Jan 1, 2025
@dariusc93 dariusc93 self-requested a review January 1, 2025 04:27
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

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

Sorry, looks like you would need to add #[allow(deprecated)] to support_draft_29 too. Could you do that too? :)

@tesol2y090 tesol2y090 requested a review from dariusc93 January 1, 2025 04:46
@@ -59,6 +59,8 @@ pub struct Config {
/// If support for draft-29 is enabled servers support draft-29 and version 1 on all
/// QUIC listening addresses.
/// As client the version is chosen based on the remote's address.
#[allow(deprecated)]
Copy link
Contributor

@elenaf9 elenaf9 Jan 4, 2025

Choose a reason for hiding this comment

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

I think the #[allow(deprecated)] needs to be applied in all the places where support_draft_29 is used, instead of here where it is declared.

Also, could we use expect instead of allow? expect will give us a warning as soon as the attribute isn't needed anymore, i.e. as soon as the deprecated items are removed.
(Depends on #5650 to update our MSRV to the most recent rust version where expect was stablilized.)

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.

quic: don't support draft versions
3 participants