Skip to content

Stream command output to logger #346

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

bdubertret
Copy link

This PR tries to fix #345 by using a TeeReader on the command stdout and stderr, and streaming the output to the log as it comes from the invoked command.

Following discution on my original PR #344, i have rewritten this PR to follow @moorereason 's advice (request to merge on development branch, remove unrelated commit)

@gamerson
Copy link

hey @bdubertret any update on this PR from project leads? I am also interested in this feature as we have long running executables that we would like to see the log.

}
stderr, err := cmd.StderrPipe()
if err != nil {
log.Fatal(err)
Copy link
Collaborator

@moorereason moorereason Apr 24, 2020

Choose a reason for hiding this comment

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

same

log.Fatal(err)
}
if err := cmd.Start(); err != nil {
log.Fatal(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

out, err := cmd.CombinedOutput()
stdout, err := cmd.StdoutPipe()
if err != nil {
log.Fatal(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't call log.Fatal. Change these to something like:

log.Printf("[%s] error opening stdout: %s", rid, err)
return "", err

Copy link
Collaborator

@moorereason moorereason left a comment

Choose a reason for hiding this comment

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

Sorry this has been sitting for so long. I like this PR and want to get it merged. However, there are merge conflicts now and a few changes I'd like to see made before we merge.

@wille
Copy link

wille commented May 3, 2021

I'd be happy to rebase so we can get this live

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants