Copilot commented on code in PR #1105:
URL:
https://github.com/apache/skywalking-banyandb/pull/1105#discussion_r3177642634
##########
bydbctl/internal/cmd/analyze.go:
##########
@@ -74,9 +74,9 @@ func newAnalyzeCmd() *cobra.Command {
if s.Subject == subjectName {
found = true
for i := range s.EntityValues {
- fmt.Printf("%s,",
pbv1.MustTagValueToStr(s.EntityValues[i]))
+
fmt.Fprintf(cmd.OutOrStdout(), "%s,", pbv1.MustTagValueToStr(s.EntityValues[i]))
}
- fmt.Println()
+ fmt.Fprintln(cmd.OutOrStdout())
Review Comment:
The new writer-based output path in the `--subject` branch isn't exercised
anywhere in `analyze_test.go`. Since this PR is specifically fixing global
stdout usage, a regression here would go unnoticed because the tests only cover
the aggregate path.
##########
bydbctl/internal/cmd/use.go:
##########
@@ -32,13 +32,13 @@ func newUseCmd() *cobra.Command {
Version: version.Build(),
Short: "Select a group",
Args: cobra.ExactArgs(1),
- RunE: func(_ *cobra.Command, args []string) (err error) {
+ RunE: func(cmd *cobra.Command, args []string) (err error) {
viper.Set("group", args[0])
err = viper.WriteConfig()
if err != nil {
return err
}
- fmt.Printf("Switched to [%s]", viper.GetString("group"))
+ fmt.Fprintf(cmd.OutOrStdout(), "Switched to [%s]",
viper.GetString("group"))
Review Comment:
`use` now depends on Cobra's output writer for the same stdout-capture fix
as the other commands, but this package still has no test covering that command
at all. A future change that writes directly to process stdout again here would
not fail any test.
##########
bydbctl/internal/cmd/root.go:
##########
@@ -123,7 +123,7 @@ func initConfig() {
return fmt.Errorf("unable to stat config file: %w", err)
}
if info.Mode().Perm() != 0o600 {
- fmt.Printf("config file %s has unsafe permissions: %o
(expected 0600)", configFile, info.Mode().Perm())
+ fmt.Fprintf(os.Stderr, "config file %s has unsafe
permissions: %o (expected 0600)\n", configFile, info.Mode().Perm())
}
// Dump this to stderr in case of mixing up response yaml
fmt.Fprintln(os.Stderr, "Using config file:", configFile)
Review Comment:
These config-file diagnostics still bypass Cobra's configured writers and go
straight to the process stderr. That means commands executed with a custom
`SetErr` writer still leak output globally when `--config` is used, so the
output-isolation change in this PR is incomplete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
