-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add compatibility with only websocket-capable clients #2132
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
cd89834
handle control protocol through websocket
enoperm 18c89c3
get DERP-over-websocket working for wasm clients
enoperm a2d5143
Prepare for testing builtin websocket-over-DERP
enoperm 73e3383
integration tests: properly differentiate between DERP transports
enoperm 6850d31
do not touch unrelated code
enoperm be08dbc
linter fixes
enoperm 6853e83
Merge remote-tracking branch 'origin/main' into websocket-control-pro…
enoperm f509a43
integration testing: unexport common implementation of derp server sc…
enoperm 4fe9ba3
fixup! integration testing: unexport common implementation of derp se…
enoperm 1a6b21a
dockertestutil/logs: remove unhelpful comment
enoperm c521e1d
update changelog
enoperm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,5 +242,4 @@ func TestValidateResolvConf(t *testing.T) { | |
| } | ||
| }) | ||
| } | ||
|
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially, not tested or was aware of it, happy for that to be changed, maybe you can test in a followup pr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested it either, I just thought maybe doing the equivalent of
> output.fileis simpler than doing an extra lap around the same thing. I don't expect any significant improvements or breakages from it either, other than the code becoming shorter. I don't mind doing an extra PR, but should it include only this spot, or all other file operations that may involve redundantio.Writers?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have time, all would of course be useful. Since this is in the integration tests, its not really that critical, but always nice to have it neater.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to look for other manually buffered disk writes with
git grep, but as far as I can see, this was the only spot where it happened.I can submit an MR with the changes, but... the code did not become much shorter. The tally comes to -5 lines, three of which were the comment I added. I could still see some value in it, because unlike
bytes.Buffer, which (at least if I remember correctly) by default extends however many times it needs to fit things into memory,bufio.Writerhas a defined buffer size and automatically flushes when it is about to cross it. So, in theory it could make it so that longer logs can be dumped without issue. I'm still waiting for the integration tests to run on github, but I do not expect problems with it.(My machine does not seem to like the idea of running the entire test suite at once too much - best I could do was 34 to go before it timed out, and I think I might have raised the timeout before reaching that result).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaand it actually failed ina few tests - must have forgot about a flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair, this isnt really critical, if it isnt much simpler or neater, it can be dropped. I'm happy either way as it currently does work, good suggestion tho. You can submit it if it seem to work, and we can take if from there but you dont have to spend that much time on it.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the comment for now. I may take another look at it, but at this point I would not really mind leaving it as it is either.