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.

Sign up for GitHub

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(cmd): set defaults for command output #4071

Merged
merged 1 commit into from
Dec 3, 2024
Merged

fix(cmd): set defaults for command output #4071

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/celestia-appd/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ func NewRootCmd() *cobra.Command {
rootCommand := &cobra.Command{
Use: "celestia-appd",
PersistentPreRunE: func(command *cobra.Command, _ []string) error {
command.SetOut(command.OutOrStdout())
command.SetErr(command.ErrOrStderr())
Comment on lines +59 to +60
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@rootulp rootulp Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 unexpected

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created #4076


clientContext, err := client.ReadPersistentCommandFlags(initClientContext, command.Flags())
if err != nil {
return err
Expand Down
Loading