Skip to content
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

Improve error output when executing step fails #325

Merged
merged 3 commits into from
Sep 22, 2020
Merged

Conversation

mrnugget
Copy link
Contributor

This is the first of possibly many stabs at improving the error
reporting.

Before

before

After

after

This is the first of possibly many stabs at improving the error
reporting.
@mrnugget mrnugget requested a review from a team September 22, 2020 13:05
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Nayse

internal/campaigns/run_steps.go Show resolved Hide resolved
Comment on lines +347 to +355
if len(e.Stdout) > 0 {
out.WriteString("\nstandard out:\n")
printOutput(e.Stdout)
}

if len(e.Stderr) > 0 {
out.WriteString("\nstandard error:\n")
printOutput(e.Stderr)
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you think they should be printed separately? I don't really know many places where that's the case (CI providers, GH actions, ..) usually print everything together (and sometimes it could be confusing I guess if:

stdout: Processing resource file X.yml
stderr: Failed to process.
stdout: Processing resource file Y.yml
stdout: Done, 1 errors.

, how would I know which of the files failed. Maybe it's just me though, so leaving it up to you and approve
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, they print everything together because they stream the output, we don't. We buffer and then print. And since we buffer them separately we can't easily weave them back together to get to your example output. We'd need to record some timing information for that, but that's a hole I don't want to jump into. Or we could use a custom entity that buffers and prefixes both.

Honestly, though, I like having them separate. I hate having to dig through lines and lines of output to get to the error messages. It's something that annoys me about CI providers actually 😄

Copy link
Member

Choose a reason for hiding this comment

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

we could use the same buffer though, no? 🤔 I mean, which tool prints simultaneously on both out and err 🤔

But if you like it better, go for it, I was just scared that some tools might use the channels in a weird way that only works when they're printed next to each other, like in my sample.
Probably an unreasonable fear though 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sure, we could use the same buffer, but then we'd just have everything together in one without knowing at a quick glance what's error or output. And considering how noisy some tools are... If we run into issues, we can revisit this, I think.

Copy link
Member

Choose a reason for hiding this comment

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

totally

@mrnugget mrnugget merged commit ad4bc3a into main Sep 22, 2020
@mrnugget mrnugget deleted the mrn/error-reporting-1 branch September 22, 2020 14:09
scjohns pushed a commit that referenced this pull request Apr 24, 2023
* Improve error output when executing step fails

This is the first of possibly many stabs at improving the error
reporting.

* Add changelog

* Remove dependency on output library in campaigns lib
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants