Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#33636 closed defect (fixed)

Remove go-webrtc dependency from snowflake

Reported by: cohosh Owned by: cohosh
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords:
Cc: cohosh, phw, arlolra, dcf Actual Points:
Parent ID: #19409 Points:
Reviewer: Sponsor:

Description

We still depend on go-webrtc because of the testing code in server-webrtc. Let's migrate this to pion or get rid of this testing code. go-webrtc is now officially unmaintained, and this will make it easier for us to package Snowflake for #19409

Child Tickets

Change History (15)

comment:1 Changed 7 months ago by cohosh

Parent ID: #19409

comment:2 Changed 7 months ago by cohosh

Status: assignedneeds_review

Here's a PR: https://github.com/cohosh/snowflake/pull/22

Note: I ran go mod tidy which made a few additional changes to go.mod. These changes seem fine.

comment:3 Changed 7 months ago by arlolra

I left a comment inline on the pull.

Separately, did you try spinning up a server and connecting to it? Also, this server is going to need updating in the turbo tunnel branches. Maybe it isn't worth maintaining going forward? We can always revive it from git history if a need crops up.

comment:4 in reply to:  3 Changed 7 months ago by cohosh

Replying to arlolra:

I left a comment inline on the pull.

Separately, did you try spinning up a server and connecting to it? Also, this server is going to need updating in the turbo tunnel branches. Maybe it isn't worth maintaining going forward? We can always revive it from git history if a need crops up.

Thanks. I did a test, yeah, and it works well. I guess I'm not too concerned with maintaining it. The main advantage as far as I can tell is to easily test things between the client and server without having to go through the deployed broker. This is made a lot easier with Snowbox, however, and if we weren't using it for testing TurboTunnel anyway then I'm not sure what the point is.

I'm all for removing it.

Last edited 7 months ago by cohosh (previous) (diff)

comment:5 Changed 7 months ago by dcf

I concur with removing server-webrtc. I haven't used it in forever.

comment:6 Changed 7 months ago by cohosh

comment:7 Changed 7 months ago by arlolra

Status: needs_reviewmerge_ready

LGTM

comment:8 Changed 7 months ago by cohosh

Resolution: fixed
Status: merge_readyclosed

Thanks! Squashed and merged at e26373dd

comment:9 Changed 7 months ago by cohosh

Resolution: fixed
Status: closedreopened

Ooops forgot to do a go mod tidy.

comment:11 Changed 7 months ago by arlolra

It might be useful if you split this into two commits since it looks like most of the changes show up on master before e26373dd was merged, but either way seems fine.

comment:12 Changed 7 months ago by arlolra

I left a comment inline on the pull.

I took a stab at the refactor suggested there in,
https://github.com/keroserene/snowflake/commit/7b761d4c8d0e56b9148f106eb01667a7ec5c0424

Can open a separate ticket for it if it's preferred.

comment:13 Changed 7 months ago by cohosh

Let's make a new ticket

comment:14 Changed 7 months ago by cohosh

Resolution: fixed
Status: needs_reviewclosed

comment:15 Changed 7 months ago by arlolra

Let's make a new ticket

See #33638

Note: See TracTickets for help on using tickets.