[Workspace CLI] better logs streaming for validate
#20238
Conversation
|
|
||
| char := buffer.Next(size) | ||
|
|
||
| if r == '\n' || r == '\r' { |
There was a problem hiding this comment.
What other (line-)control characters would we need to support as well? (wikipedia). And: Do we need to handle the windows line ending (/r/n) as well? 🤔
Otherwise, I think this is a nice approach navigating the difficulties we discussed yesterday, especially in the face of "merging multiple streams". 💪
There was a problem hiding this comment.
Also, would it make sense to add (minimal) tests for this decoding/forwarding logic? I imagine extracting a the core function would help a lot.
Also2, 1-2 lines of comments on "why this approach", especially vs. "just forward byte stream", would be 🥇 .
There was a problem hiding this comment.
Totally agree about the tests and comment-adding. Took a look at the Wikipedia page you linked, and it looks like only \b could be interesting for us (in addition to \r and \t?
Added changes in f87e49e (#20238)
| return | ||
| } | ||
| flushLine := func() { | ||
| if line.Len() > 0 { |
There was a problem hiding this comment.
this has a catch.
Pros:
- When we read Windows-like line endings (
\r\n), we treat it as a single line-break
Cons:
- A hundred line breaks will be treated as 0 if the line has no other content
Description
Makes log streaming not utilize any fixed-size buffers, and instead accepts not just
\n, but also\ras a newline character.Related Issue(s)
Fixes ENT-629
How to test
You can test logging by cloning a repo under
/workspaceand executing something along the lines of the following (fromcomponents/gitpod-cli):A test of some common scenarios (such as long line entries, multibyte Unicode characters and same-line updates) can be tested by running this on https://github.com/filiptronicek/test-foofies/tree/ft/log.