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(stderr): ensure we output error to stderr, not stdout #1074

Closed
wants to merge 3 commits into from

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Nov 1, 2023

This updates the cprint package to explicitly specify the output stream.

All occurrences have been updated accordingly, explicit warnings are now going to stderr. A few statements remain to stdout.

Is this breaking?

  • it changes signatures in the internal helper package cprint, is this part of the external API?
  • the Syncer (see changes in diff/diff.go) remains backward compatible, since that is part of the external API.

@Tieske Tieske requested a review from a team November 1, 2023 15:05
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2023

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (d110df8) 32.87% compared to head (74fe260) 32.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1074      +/-   ##
==========================================
- Coverage   32.87%   32.87%   -0.01%     
==========================================
  Files         103      103              
  Lines       12847    12850       +3     
==========================================
  Hits         4224     4224              
- Misses       8214     8217       +3     
  Partials      409      409              
Files Coverage Δ
convert/convert.go 82.08% <100.00%> (ø)
cmd/gateway_diff.go 0.00% <0.00%> (ø)
cmd/gateway_dump.go 0.00% <0.00%> (ø)
cmd/gateway_ping.go 0.00% <0.00%> (ø)
cmd/gateway_reset.go 0.00% <0.00%> (ø)
cmd/gateway_sync.go 0.00% <0.00%> (ø)
cmd/gateway_validate.go 0.00% <0.00%> (ø)
utils/utils.go 25.61% <0.00%> (ø)
cmd/konnect.go 0.00% <0.00%> (ø)
cprint/color.go 82.85% <88.88%> (ø)
... and 3 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GGabriele
Copy link
Collaborator

is this part of the external API?

Yes, this is used by KIC

@Kong/team-k8s

@GGabriele GGabriele requested a review from a team November 1, 2023 17:45
@mheap
Copy link
Member

mheap commented Nov 1, 2023

I think this would be a breaking change for people that expect warnings on stdout for older commands and have scripts that rely on this output.

I'd expect the following behaviour:

  • Deprecation notices that were added prior to 1.28 remain on stdout
  • Deprecation notices added in 1.28 are sent to stderr

@rainest
Copy link
Contributor

rainest commented Nov 1, 2023

I don't think we care much on the KIC side. We disable deck logs above debug and cannot parse them.

I don't think the changes other than those in diff/diff.go would affect us, since we don't use the cmd package and instead build our own Syncer instance. As of this comment those don't actually appear to change anything (they're still going to stdout with a TODO to change them later). stderr is correct for them (they are informational messages, not output).

The multi-line diff output KIC gets isn't really suitable for log daemon consumption either way, so I'd be fine changing them immediately. It's breaking in the strictest sense, but not really given the practical use case (a human looks at container logs directly for debugging) those messages fit at present.


One of the limitations of our current deck interface for KIC is that deck doesn't let you inject a logger, as much of deck doesn't actually use a log library--the cprint stuff is just a basic wrapper around fmt.Println() with a very basic notion of verbosity.

The lines KIC ends up showing are logged within the Syncer rather than returned from it, which is one of the things we'd want to change if moving to a more library-oriented design. For now we only have the global cprint on/off toggle.

@Tieske
Copy link
Member Author

Tieske commented Nov 2, 2023

closing this since it is considered breaking

@Tieske Tieske closed this Nov 2, 2023
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.

5 participants