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.

Sign up for GitHub

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

Jump to bottom

Add content for QUIC #191

Merged
merged 18 commits into from
Oct 7, 2022
Merged

Add content for QUIC #191

merged 18 commits into from
Oct 7, 2022

Conversation

salmad3
Copy link
Member

@salmad3 salmad3 commented Sep 14, 2022 •
edited
Loading

Context

Latest preview

Please view the latest Fleek preview here.

@salmad3 salmad3 marked this pull request as draft September 14, 2022 05:38
@salmad3 salmad3 changed the title Add content for QUIC Add content for QUIC & WebTransport Sep 16, 2022
@salmad3 salmad3 marked this pull request as ready for review September 16, 2022 09:07
@salmad3 salmad3 added the ready for review PR is ready for review label Sep 16, 2022
@mxinden
Copy link
Member

mxinden commented Sep 16, 2022

//CC @elenaf9 given that you own libp2p/rust-libp2p#2883.

MarcoPolo
MarcoPolo requested changes Sep 21, 2022
View reviewed changes
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
@salmad3 salmad3 added change request PR requires changes. and removed ready for review PR is ready for review labels Sep 21, 2022
@salmad3 salmad3 added the ready for review PR is ready for review label Sep 21, 2022
@salmad3
Copy link
Member Author

salmad3 commented Sep 21, 2022

Thanks @MarcoPolo! Suggestions and feedback incorporated

marten-seemann
marten-seemann reviewed Sep 21, 2022
View reviewed changes
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

I did a first pass over the QUIC section. Haven’t touched WebTransport yet.

Maybe we can split this PR and first get QUIC merged, and then tackle WebTransport?

content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
@salmad3 salmad3 changed the title Add content for QUIC & WebTransport Add content for QUIC Sep 21, 2022
@salmad3 salmad3 mentioned this pull request Sep 21, 2022
Merged
@salmad3
Copy link
Member Author

salmad3 commented Sep 21, 2022

@marten-seemann thanks! Incorporated the suggestions and PRs now stacked

elenaf9
elenaf9 reviewed Sep 21, 2022
View reviewed changes
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
salmad3 and others added 3 commits September 21, 2022 11:22
@salmad3 @elenaf9
Apply suggestions from code review
b76e2b3 
Co-authored-by: Elena Frank <elena.frank@protonmail.com>
@salmad3 @elenaf9
Update content/concepts/transport.md
552dd99 
Co-authored-by: Elena Frank <elena.frank@protonmail.com>
@salmad3
address PR feedback; rmv notices
b375ec2
@salmad3
Copy link
Member Author

salmad3 commented Sep 21, 2022

Thanks @elenaf9! Incorporated suggestions

marten-seemann
marten-seemann reviewed Sep 23, 2022
View reviewed changes
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

I think it would be valuable to add the OSI layer diagram in this PR. We'll probably have to word-smith the text accompanying that diagram a bit, but overall, I like where this PR is going :)

content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
@salmad3 salmad3 removed the change request PR requires changes. label Sep 24, 2022
marten-seemann
marten-seemann reviewed Sep 28, 2022
View reviewed changes
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

This is beginning to look really good. A few more comments.

content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
content/concepts/transport.md Outdated Show resolved Hide resolved
marten-seemann
marten-seemann reviewed Sep 30, 2022
View reviewed changes
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Looks like there's still 2 unaddressed comments. Did you miss those @DannyS03?

@salmad3 @marten-seemann
Apply suggestions from code review
74d97dd 
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
@salmad3
Copy link
Member Author

salmad3 commented Oct 4, 2022

@marten-seemann checking if there are any other parts to reconsider

@salmad3 salmad3 removed the ready for review PR is ready for review label Oct 7, 2022
@salmad3 salmad3 merged commit 01313f1 into master Oct 7, 2022
@salmad3 salmad3 deleted the transport/quic branch January 3, 2023 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcoPolo MarcoPolo MarcoPolo approved these changes

@marten-seemann marten-seemann marten-seemann approved these changes

@elenaf9 elenaf9 Awaiting requested review from elenaf9

Assignees
No one assigned
Labels
None yet
Projects
go-libp2p
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add QUIC to transport documentation
5 participants
@salmad3 @mxinden @MarcoPolo @marten-seemann @elenaf9