fix(cmd): set defaults for command output #4071
Conversation
rootulp
requested review from
rach-id and
evan-forbes
and removed request for
a team
📝 WalkthroughWalkthroughThe changes in this pull request modify the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (2)cmd/celestia-appd/cmd/root.go (2)
Since this change is in ✅ Verification successfulLet me analyze the results and generate the final response. Setting command output is safe across all subcommands The search results show that the only direct usage of
There's no evidence of subcommands relying on custom output handling that would be affected by this change. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any commands that might override or depend on custom output handling
# Look for any custom output handling in commands
rg -A 3 'command\.(Out|Err|OutOrStdout|ErrOrStderr)' --type go
# Check for any direct writes to os.Stdout or os.Stderr
rg -A 3 'os\.(Stdout|Stderr)' --type go
Length of output: 3026
The change appropriately uses Cobra's built-in methods to set output streams, which should fix the stderr redirection issue for all commands including Let's verify that this change fixes the specific issue: ✅ Verification successfulThe output stream configuration is correctly implemented and won't be overridden The verification shows that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that the status command output goes to stdout
# Note: This script helps verify the fix by checking the command implementation
# Check if the status command is properly configured
ast-grep --pattern 'rpc.StatusCommand()'
# Verify there are no other places overriding the output stream for status command
rg -l 'SetOut|SetErr' --type go
Length of output: 208 Script: #!/bin/bash
# Let's check the content of addr_converter_test.go to ensure it doesn't override our settings
cat cmd/celestia-appd/cmd/addr_converter_test.go
# Also check the actual usage of SetOut/SetErr in root.go for completeness
rg 'SetOut|SetErr' --type go -A 2 -B 2
Length of output: 2359 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| command.SetOut(command.OutOrStdout()) | ||
| command.SetErr(command.ErrOrStderr()) |
There was a problem hiding this comment.
does this change all logs to go to the output instead of the error? if so, is a breaking change for things that collect logs?
There was a problem hiding this comment.
if I understand correctly, this is just for the prints. the logs are handled separatly:
var logWriter io.Writer
if strings.ToLower(serverCtx.Viper.GetString(flags.FlagLogFormat)) == tmcfg.LogFormatPlain {
logWriter = zerolog.ConsoleWriter{Out: os.Stderr}
} else {
logWriter = os.Stderr
}
logLvlStr := serverCtx.Viper.GetString(flags.FlagLogLevel)
logLvl, err := zerolog.ParseLevel(logLvlStr)
if err != nil {
return fmt.Errorf("failed to parse log level (%s): %w", logLvlStr, err)
}
serverCtx.Logger = ZeroLogWrapper{zerolog.New(logWriter).Level(logLvl).With().Timestamp().Logger()}
Rootul can correct me
There was a problem hiding this comment.
does this change all logs to go to the output instead of the error?
Output that targets stderr will still go to stderr. This fixes an issue where commands that used Cobra's
// Println is a convenience method to Println to the defined output, fallback to Stderr if not set.
func (c *Command) Println(i ...interface{}) {
c.Print(fmt.Sprintln(i...))
}would previously go to stderr because stdout wasn't configured for the command. The status command is an example that used this Println method
There was a problem hiding this comment.
nice, makes sense
it might still be a bit confusing to anyone reading the code, as the logs are set to go to stderr in the sdk
There was a problem hiding this comment.
as the logs are set to go to stderr in the sdk
huh that seems unexpected. I'll try running a node pre / post this change to double check that I didn't break something
There was a problem hiding this comment.
wow you're right. the SDK does send logs to STDERR. Before (and after) this change, error.txt contains logs from the node:
./scripts/single-node.sh > output.txt 2> error.txt
# output.txt contains output from the script
# error.txt contains logs from the node which is unexpectedThere was a problem hiding this comment.
Given that behavior is related but existed before this change, I'm inclined to merge this as a fix for the linked issue and create a new one for this weird behavior.
evan-forbes
added
the
backport:v3.x
Closes #3823 Motivation celestiaorg/cosmos-sdk#420 (comment) ## Testing ```bash make build ./build/celestia-appd status | jq . ``` works (cherry picked from commit 615f1dc)
Closes #3823 Motivation celestiaorg/cosmos-sdk#420 (comment) ## Testing ```bash make build ./build/celestia-appd status | jq . ``` works <hr>This is an automatic backport of pull request #4071 done by [Mergify](https://mergify.com). Co-authored-by: Rootul P <[email protected]>
Closes #3823
Motivation celestiaorg/cosmos-sdk#420 (comment)
Testing
works