Remove custom WebSocket code #4001

BlackDex · 2023-10-25T14:52:29Z

Remove our custom WebSocket code and only use the Rocket code. Removed all options in regards to WebSockets
Added a new option WEBSOCKET_DISABLED which defaults too false. This can be used to disable WebSockets if you really do not want to use it.

Also:

Updated crates, including Rocket to the latest merged v0.5 changes
Removed an extra header which should not be sent for websocket connections

stefan0xC

Looks mostly good to me. Not sure I'm a fan of the new configuration option name though.

If we don't want to keep the old name (for which I guess there are reasons, i.e. some users would have to change their configuration if they have set the value to false explicitly or saved a data/config.json with the old default), I'd rather call it WEBSOCKETS_ENABLED instead. I think it would be better to have an affirmative name for booleans as they tend to be easier to reason about (i.e. a feature is enabled when it's true, not when a disable toggle is on).

If we want to communicate that this is an option to disable the feature (to discourage the use) I think we should use the active form DISABLE_WEBSOCKETS instead (as we do with a few other configuration options`).

.env.template

src/api/notifications.rs

tessus · 2023-11-01T15:58:35Z

~~The way this is implemented now means that this is a breaking change. If vw follows semantic versioning, the next release will have to be 2.0.0.~~

~~Even if vw didn't follow semantic versioning, a proper BREAKING changelog entry should be added.~~

Update: I was mistaken. I thought to recall that non existent options in the config file will abend the start of the server. But this is not the case. I have tested it just now. Please ignore my comment.

src/api/admin.rs

SergioBenitez

Nothing too interesting. Looks like a straightforward migration to rc4!

src/api/notifications.rs

tessus · 2024-02-02T23:04:57Z

Can this be merged? I think it makes sense to have this "break" reverse-proxy settings in testing before the 1.31.0 is released.

BlackDex · 2024-02-02T23:14:01Z

We (might) need to release a 1.30.4 first.
Because of #4307, which is fixed now.

But after that (if nothing else pops up), that was the plan.

Remove our custom WebSocket code and only use the Rocket code. Removed all options in regards to WebSockets Added a new option `WEBSOCKET_DISABLED` which defaults too `false`. This can be used to disable WebSockets if you really do not want to use it.

- Addressed comments given during review - Updated crates, including Rocket to the latest merged v0.5 changes - Removed an extra header which should not be sent for websocket connections

- Addressed the suggestions - Updated Rocket to latest rc4 Also made the needed code changes - Updated all other crates Pinned `openssl` and `openssl-sys`

tessus · 2024-03-14T23:11:57Z

Ping. ;-) Can a few PRs be merged? The list of PRs id getting longer and longer...

* Remove custom WebSocket code Remove our custom WebSocket code and only use the Rocket code. Removed all options in regards to WebSockets Added a new option `WEBSOCKET_DISABLED` which defaults too `false`. This can be used to disable WebSockets if you really do not want to use it. * Addressed remarks given and some updates - Addressed comments given during review - Updated crates, including Rocket to the latest merged v0.5 changes - Removed an extra header which should not be sent for websocket connections * Updated suggestions and crates - Addressed the suggestions - Updated Rocket to latest rc4 Also made the needed code changes - Updated all other crates Pinned `openssl` and `openssl-sys` --------- Co-authored-by: Daniel García <dani-garcia@users.noreply.github.com>

BlackDex force-pushed the remove-custom-websocket-implementation branch from 9214e2d to a607bcc Compare October 25, 2023 14:54

stefan0xC suggested changes Oct 26, 2023

View reviewed changes

.env.template Outdated Show resolved Hide resolved

src/api/notifications.rs Outdated Show resolved Hide resolved

src/api/notifications.rs Outdated Show resolved Hide resolved

BlackDex requested a review from stefan0xC October 27, 2023 15:54

SergioBenitez reviewed Nov 1, 2023

View reviewed changes

src/api/admin.rs Outdated Show resolved Hide resolved

SergioBenitez reviewed Nov 1, 2023

View reviewed changes

src/api/admin.rs Outdated Show resolved Hide resolved

BlackDex requested a review from SergioBenitez November 3, 2023 12:11

BlackDex mentioned this pull request Nov 3, 2023

⚠️[NOTE] BREAKING CHANGE sinces v1.31.0 #4024

Open

SergioBenitez reviewed Nov 6, 2023

View reviewed changes

src/api/notifications.rs Show resolved Hide resolved

BlackDex force-pushed the remove-custom-websocket-implementation branch from 2b63ec1 to c4e0f31 Compare November 15, 2023 10:37

BlackDex force-pushed the remove-custom-websocket-implementation branch from c4e0f31 to fed129f Compare December 15, 2023 15:55

BlackDex force-pushed the remove-custom-websocket-implementation branch from fed129f to ae8a535 Compare January 13, 2024 17:22

BlackDex force-pushed the remove-custom-websocket-implementation branch 6 times, most recently from 840ffe9 to f4ee2d3 Compare January 31, 2024 22:20

BlackDex force-pushed the remove-custom-websocket-implementation branch 2 times, most recently from 312b6b1 to 24c5711 Compare February 2, 2024 22:42

BlackDex force-pushed the remove-custom-websocket-implementation branch 2 times, most recently from cd3d8a0 to 6fa38b2 Compare February 9, 2024 16:13

BlackDex added 3 commits February 16, 2024 15:59

Remove custom WebSocket code

f44fec3

Remove our custom WebSocket code and only use the Rocket code. Removed all options in regards to WebSockets Added a new option `WEBSOCKET_DISABLED` which defaults too `false`. This can be used to disable WebSockets if you really do not want to use it.

Addressed remarks given and some updates

917fb50

- Addressed comments given during review - Updated crates, including Rocket to the latest merged v0.5 changes - Removed an extra header which should not be sent for websocket connections

Updated suggestions and crates

d6cbbc3

- Addressed the suggestions - Updated Rocket to latest rc4 Also made the needed code changes - Updated all other crates Pinned `openssl` and `openssl-sys`

BlackDex force-pushed the remove-custom-websocket-implementation branch from 6fa38b2 to d6cbbc3 Compare February 16, 2024 15:00

Merge branch 'main' into remove-custom-websocket-implementation

6232700

dani-garcia approved these changes Mar 17, 2024

View reviewed changes

dani-garcia merged commit 3427217 into dani-garcia:main Mar 17, 2024
5 checks passed

BlackDex deleted the remove-custom-websocket-implementation branch March 17, 2024 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove custom WebSocket code #4001

Remove custom WebSocket code #4001

BlackDex commented Oct 25, 2023 •

edited

Loading

stefan0xC left a comment

tessus commented Nov 1, 2023 •

edited

Loading

SergioBenitez left a comment

tessus commented Feb 2, 2024

BlackDex commented Feb 2, 2024

tessus commented Mar 14, 2024

Remove custom WebSocket code #4001

Remove custom WebSocket code #4001

Conversation

BlackDex commented Oct 25, 2023 • edited Loading

stefan0xC left a comment

Choose a reason for hiding this comment

tessus commented Nov 1, 2023 • edited Loading

SergioBenitez left a comment

Choose a reason for hiding this comment

tessus commented Feb 2, 2024

BlackDex commented Feb 2, 2024

tessus commented Mar 14, 2024

BlackDex commented Oct 25, 2023 •

edited

Loading

tessus commented Nov 1, 2023 •

edited

Loading