Skip to content

[Workspace CLI] better logs streaming for validate #20238

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

Merged
merged 2 commits into from
Sep 24, 2024
Merged

[Workspace CLI] better logs streaming for validate #20238

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a626ad1
[Workspace CLI] better logs streaming for `validate`
filiptronicek Sep 24, 2024
f87e49e
add tests, handle `\b`
filiptronicek Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 40 additions & 38 deletions components/gitpod-cli/cmd/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package cmd

import (
"bufio"
"bytes"
"context"
"encoding/json"
"fmt"
Expand All @@ -16,6 +17,7 @@ import (
"path/filepath"
"strings"
"time"
"unicode/utf8"

"github.com/gitpod-io/gitpod/common-go/log"
"github.com/gitpod-io/gitpod/gitpod-cli/pkg/supervisor"
Expand Down Expand Up @@ -565,58 +567,58 @@ func pipeTask(ctx context.Context, task *api.TaskStatus, supervisor *supervisor.
}

func listenTerminal(ctx context.Context, task *api.TaskStatus, supervisor *supervisor.SupervisorClient, runLog *logrus.Entry) error {
listen, err := supervisor.Terminal.Listen(ctx, &api.ListenTerminalRequest{
Alias: task.Terminal,
})
listen, err := supervisor.Terminal.Listen(ctx, &api.ListenTerminalRequest{Alias: task.Terminal})
if err != nil {
return err
}

pr, pw := io.Pipe()
defer pr.Close()
defer pw.Close()
var buffer, line bytes.Buffer

scanner := bufio.NewScanner(pr)
const maxTokenSize = 1 * 1024 * 1024 // 1 MB
buf := make([]byte, maxTokenSize)
scanner.Buffer(buf, maxTokenSize)
flushLine := func() {
if line.Len() > 0 {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

runLog.Infof("%s: %s", task.Presentation.Name, line.String())
line.Reset()
}
}

go func() {
defer pw.Close()
for {
resp, err := listen.Recv()
if err != nil {
_ = pw.CloseWithError(err)
return
for {
resp, err := listen.Recv()
if err != nil {
if err == io.EOF {
flushLine()
return nil
}
return err
}

title := resp.GetTitle()
if title != "" {
task.Presentation.Name = title
}
if title := resp.GetTitle(); title != "" {
task.Presentation.Name = title
}

exitCode := resp.GetExitCode()
if exitCode != 0 {
runLog.Infof("%s: exited with code %d", task.Presentation.Name, exitCode)
}
if exitCode := resp.GetExitCode(); exitCode != 0 {
flushLine()
runLog.Infof("%s: exited with code %d", task.Presentation.Name, exitCode)
}

data := resp.GetData()
if len(data) > 0 {
_, err := pw.Write(data)
if err != nil {
_ = pw.CloseWithError(err)
return
if data := resp.GetData(); len(data) > 0 {
buffer.Write(data)

for {
r, size := utf8.DecodeRune(buffer.Bytes())
if r == utf8.RuneError && size == 0 {
break // incomplete character at the end
}

char := buffer.Next(size)

if r == '\n' || r == '\r' {
Copy link
Member

Choose a reason for hiding this comment

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

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". 💪

Copy link
Member

Choose a reason for hiding this comment

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

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 🥇 .

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

flushLine()
} else {
line.Write(char)
}
}
}
}()

for scanner.Scan() {
line := scanner.Text()
runLog.Infof("%s: %s", task.Presentation.Name, line)
}

return scanner.Err()
}

var validateOpts struct {
Expand Down
Loading