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

fix(#2123): Move output to stderr #2125

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

jzorn
Copy link
Contributor

@jzorn jzorn commented Apr 18, 2024

Description

This change moves informational command outputs from console.log (prints on stdout) to console.warn/console.error (prints on stderr) to enable stdout processing in pipelines.

I am not sure how to properly test if this breaks anything, if I have missed anything I am happy to fix any problems.

Fixes #2123

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

This change moves informational command outputs from console.log (prints on stdout) to console.warn/console.error (prints on stderr)
to enable stdout processing in pipelines.
@jzorn jzorn changed the title fix: Move output to stderr #2123 fix(#2123): Move output to stderr Apr 18, 2024
@jzorn
Copy link
Contributor Author

jzorn commented Apr 22, 2024

Hey, is there anything I can do to improve this PR to get it merged? :)

@jzorn
Copy link
Contributor Author

jzorn commented Apr 25, 2024

Friendly ping :)

@jzorn
Copy link
Contributor Author

jzorn commented Apr 30, 2024

Do you happen to have feedback on this one too, @andreassiegel? Your feedback on the one was valuable to me! Thanks!

@andreassiegel
Copy link
Contributor

Hi @jzorn,
I had mixed feelings regarding the proposed change at first:

While I absolutely understand and like the idea of being able to build up command chains and to pipe the output into other tools like jq, I'm not sure about the idea of logging everything else to stderr since it actually isn't about errors.

So, to me, this felt like a "misuse" of output streams due to the conflict between "actual" (result) output and intermediate log information. This relates to my personal work context as a backend engineer: I'm trying to be careful about log levels and want to make conscious decisions about when to use which log level (although there may be reasons to do things differently on client side).

For this reason, a few outputs feel odd here:

  • writing the information about recursive runs to the warning log (line 408)
  • writing the run summary or output status to the error log (lines 472 and 495)

You see, my concerns are more about log levels than about output streams. However, both are related. Personally, I'd only use the stderr for actual error information but I also see that stderr could be used for diagnostic information, and you could consider all intermediate log output as such diagnostic information.

On the other hand, what we actually may want to have is three different output streams:

  • standard output for log information in happy cases
  • error output in case of failure
  • result output for further processing

Looking at the Bru CLI Overview, this seems to be possible with the -o or --output flags. So I think something like this would be possible without any changes:

bru run folder --output results.json
echo "results.json" | jq

So, to make a long story short:

I'm not sure whether the proposed changes are necessary or if they add more confusion.
I think this requires a general (opinionated) decision about the usage of output streams, and with the flag to write output to a dedicated file this decision probably has been made already.

Personally, I also like the idea of having the output available as a separate file that I could persist as a pipeline artifact for later inspection. So this actually opens more option for further processing than just being able to pipe the command output into some other command.

@jzorn
Copy link
Contributor Author

jzorn commented May 2, 2024

to stderr since it actually isn't about errors.

I don't think we should take the name of stderr too literal - I can't think of an application I use that would log warnings to stdout. Kubernetes comes to mind.

I am of course aware of the output file option in bruno cli, but it breaks (or hinders) building proper pipelines (see also this answer and the linked article https://stackoverflow.com/a/41766832). Something along the lines of bru run foo --output json | jq -r ".something.somethingElse" | cut -d" " -f2 | sort | uniq. Cleanup of temporary files is another burden in the current solution I'd like to avoid.
Without proper separation of actual output and "debug information", the bru cli is limited in it's value for scripting.

@andreassiegel
Copy link
Contributor

andreassiegel commented May 2, 2024

Thanks a lot for that link to the Stack Overflow issue! The referenced article was also a good read.

Also, Google doesn't really have a suggestion in their Shell style guide.

With that in mind, I'm fully with you, although the output file would be a bit redundant as it would basically do the same as redirecting the output to a file.

@jzorn
Copy link
Contributor Author

jzorn commented May 2, 2024

I think removing the output file may cause a backwards-incompatibility though. What do you think? Maybe it could be considered for v2 of the cli?

@andreassiegel
Copy link
Contributor

Yeah, I would not remove it, just accept the redundancy and maybe mark it as deprecated at some point.

It doesn't relly make a difference in the end, but for some users it might be more convenient to be able to specify an output file as an argument instead of redirecting the output stream.

@jzorn
Copy link
Contributor Author

jzorn commented May 3, 2024

@helloanoop Would you like to weigh in on this? Anything else I/we can do to get those PRs merged? Thanks! :)

@jzorn
Copy link
Contributor Author

jzorn commented May 10, 2024

Anything? :)

@jzorn
Copy link
Contributor Author

jzorn commented May 14, 2024

Friendly ping :)

@jzorn
Copy link
Contributor Author

jzorn commented May 21, 2024

Is there anything I missed? Do I need to follow a different process to get a change merged?

@jzorn
Copy link
Contributor Author

jzorn commented Jun 4, 2024

Anything @helloanoop ?

@lohxt1 lohxt1 requested review from lohxt1 and removed request for lohxt1 June 5, 2024 10:06
@helloanoop helloanoop merged commit 1b4d9b8 into usebruno:main Jun 5, 2024
3 checks passed
helloanoop added a commit that referenced this pull request Jun 5, 2024
helloanoop added a commit that referenced this pull request Jun 5, 2024
@helloanoop
Copy link
Contributor

Hey @jzorn

I had to revert the merge, Need some more time to review this,

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.

cli: add the ability to output to /dev/stdout
4 participants