diff --git a/cliext/flags.gen.go b/cliext/flags.gen.go index 28668a3ed..c607e4c13 100644 --- a/cliext/flags.gen.go +++ b/cliext/flags.gen.go @@ -38,8 +38,8 @@ func (v *CommonOptions) BuildFlags(f *pflag.FlagSet) { f.StringVar(&v.Profile, "profile", "", "Profile to use for config file. EXPERIMENTAL.") f.BoolVar(&v.DisableConfigFile, "disable-config-file", false, "If set, disables loading environment config from config file. EXPERIMENTAL.") f.BoolVar(&v.DisableConfigEnv, "disable-config-env", false, "If set, disables loading environment config from environment variables. EXPERIMENTAL.") - v.LogLevel = NewFlagStringEnum([]string{"debug", "info", "warn", "error", "never"}, "info") - f.Var(&v.LogLevel, "log-level", "Log level. Default is \"info\" for most commands and \"warn\" for \"server start-dev\". Accepted values: debug, info, warn, error, never.") + v.LogLevel = NewFlagStringEnum([]string{"debug", "info", "warn", "error", "never"}, "never") + f.Var(&v.LogLevel, "log-level", "Log level. Default is \"never\" for most commands and \"warn\" for \"server start-dev\". Accepted values: debug, info, warn, error, never.") v.LogFormat = NewFlagStringEnum([]string{"text", "json", "pretty"}, "text") f.Var(&v.LogFormat, "log-format", "Log format. Accepted values: text, json.") v.Output = NewFlagStringEnum([]string{"text", "json", "jsonl", "none"}, "text") diff --git a/cliext/option-sets.yaml b/cliext/option-sets.yaml index c8472fd03..e7a7ff8cf 100644 --- a/cliext/option-sets.yaml +++ b/cliext/option-sets.yaml @@ -51,8 +51,8 @@ option-sets: - never description: | Log level. - Default is "info" for most commands and "warn" for "server start-dev". - default: info + Default is "never" for most commands and "warn" for "server start-dev". + default: never - name: log-format type: string-enum description: Log format. diff --git a/internal/temporalcli/commands.config.go b/internal/temporalcli/commands.config.go index 79467328d..e8a423dd0 100644 --- a/internal/temporalcli/commands.config.go +++ b/internal/temporalcli/commands.config.go @@ -325,7 +325,6 @@ func writeEnvConfigFile(cctx *CommandContext, conf *envconfig.ClientConfig) erro } // Write to file, making dirs as needed - cctx.Logger.Info("Writing config file", "file", configFile) if err := os.MkdirAll(filepath.Dir(configFile), 0700); err != nil { return fmt.Errorf("failed making config file parent dirs: %w", err) } else if err := os.WriteFile(configFile, b, 0600); err != nil { diff --git a/internal/temporalcli/commands.env.go b/internal/temporalcli/commands.env.go index d98c06f04..7ec952327 100644 --- a/internal/temporalcli/commands.env.go +++ b/internal/temporalcli/commands.env.go @@ -13,7 +13,7 @@ import ( func (c *TemporalEnvCommand) envNameAndKey(cctx *CommandContext, args []string, keyFlag string) (string, string, error) { if len(args) > 0 { - cctx.Logger.Warn("Arguments to env commands are deprecated; please use --env and --key (or -k) instead") + fmt.Fprintln(cctx.Options.Stderr, "Warning: Arguments to env commands are deprecated; please use --env and --key (or -k) instead") if c.Parent.Env != "default" || keyFlag != "" { return "", "", fmt.Errorf("cannot specify both an argument and flags; please use flags instead") @@ -49,10 +49,8 @@ func (c *TemporalEnvDeleteCommand) run(cctx *CommandContext, args []string) erro env, _ := cctx.DeprecatedEnvConfigValues[envName] // User can remove single flag or all in env if key != "" { - cctx.Logger.Info("Deleting env property", "env", envName, "property", key) delete(env, key) } else { - cctx.Logger.Info("Deleting env", "env", env) delete(cctx.DeprecatedEnvConfigValues, envName) } return writeDeprecatedEnvConfigToFile(cctx) @@ -129,7 +127,6 @@ func (c *TemporalEnvSetCommand) run(cctx *CommandContext, args []string) error { if cctx.DeprecatedEnvConfigValues[envName] == nil { cctx.DeprecatedEnvConfigValues[envName] = map[string]string{} } - cctx.Logger.Info("Setting env property", "env", envName, "property", key, "value", value) cctx.DeprecatedEnvConfigValues[envName][key] = value return writeDeprecatedEnvConfigToFile(cctx) } @@ -138,7 +135,6 @@ func writeDeprecatedEnvConfigToFile(cctx *CommandContext) error { if cctx.Options.DeprecatedEnvConfig.EnvConfigFile == "" { return fmt.Errorf("unable to find place for env file (unknown HOME dir)") } - cctx.Logger.Info("Writing env file", "file", cctx.Options.DeprecatedEnvConfig.EnvConfigFile) return writeDeprecatedEnvConfigFile(cctx.Options.DeprecatedEnvConfig.EnvConfigFile, cctx.DeprecatedEnvConfigValues) } diff --git a/internal/temporalcli/commands.env_test.go b/internal/temporalcli/commands.env_test.go index cdd1157a9..f92506114 100644 --- a/internal/temporalcli/commands.env_test.go +++ b/internal/temporalcli/commands.env_test.go @@ -2,6 +2,7 @@ package temporalcli_test import ( "os" + "strings" "testing" "gopkg.in/yaml.v3" @@ -104,3 +105,32 @@ func TestEnv_InputValidation(t *testing.T) { res = h.Execute("env", "set", "myenv1.foo") h.ErrorContains(res.Err, `no value provided`) } + +func TestEnv_DeprecationWarningBypassesLogger(t *testing.T) { + h := NewCommandHarness(t) + defer h.Close() + + tmpFile, err := os.CreateTemp("", "") + h.NoError(err) + h.Options.DeprecatedEnvConfig.EnvConfigFile = tmpFile.Name() + defer os.Remove(h.Options.DeprecatedEnvConfig.EnvConfigFile) + res := h.Execute("env", "set", "--env", "myenv1", "-k", "foo", "-v", "bar") + h.NoError(res.Err) + + // Using deprecated argument syntax should produce a warning on stderr. + // The warning must bypass the logger so it appears regardless of log level. + for _, logLevel := range []string{"never", "error", "info"} { + t.Run("log-level="+logLevel, func(t *testing.T) { + res = h.Execute("env", "get", "--log-level", logLevel, "myenv1") + h.NoError(res.Err) + + stderr := res.Stderr.String() + h.Contains(stderr, "Warning:") + h.Contains(stderr, "deprecated") + + // Must be a plain-text warning, not a structured log message + h.False(strings.Contains(stderr, "time="), "warning should not be a structured log message") + h.False(strings.Contains(stderr, "level="), "warning should not be a structured log message") + }) + } +} diff --git a/internal/temporalcli/commands.gen.go b/internal/temporalcli/commands.gen.go index 1b1f31295..2455cf233 100644 --- a/internal/temporalcli/commands.gen.go +++ b/internal/temporalcli/commands.gen.go @@ -2117,7 +2117,7 @@ func NewTemporalServerStartDevCommand(cctx *CommandContext, parent *TemporalServ s.Command.Flags().StringVar(&s.UiCodecEndpoint, "ui-codec-endpoint", "", "UI remote codec HTTP endpoint.") s.Command.Flags().StringArrayVar(&s.SqlitePragma, "sqlite-pragma", nil, "SQLite pragma statements in \"PRAGMA=VALUE\" format.") s.Command.Flags().StringArrayVar(&s.DynamicConfigValue, "dynamic-config-value", nil, "Dynamic configuration value using `KEY=VALUE` pairs. Keys must be identifiers, and values must be JSON values. For example: `YourKey=\"YourString\"` Can be passed multiple times.") - s.Command.Flags().BoolVar(&s.LogConfig, "log-config", false, "Log the server config to stderr.") + s.Command.Flags().BoolVar(&s.LogConfig, "log-config", false, "Print the server config to stderr.") s.Command.Flags().StringArrayVar(&s.SearchAttribute, "search-attribute", nil, "Search attributes to register using `KEY=VALUE` pairs. Keys must be identifiers, and values must be the search attribute type, which is one of the following: Text, Keyword, Int, Double, Bool, Datetime, KeywordList.") s.Command.Run = func(c *cobra.Command, args []string) { if err := s.run(cctx, args); err != nil { diff --git a/internal/temporalcli/commands.go b/internal/temporalcli/commands.go index a60307b83..fb76546e2 100644 --- a/internal/temporalcli/commands.go +++ b/internal/temporalcli/commands.go @@ -48,7 +48,9 @@ type CommandContext struct { FlagsWithEnvVars []*pflag.Flag // These values may not be available until after pre-run of main command - Printer *printer.Printer + Printer *printer.Printer + // The logger is for the SDK and server. The CLI itself should print warnings or errors to + // stderr but should not use logging. Logger *slog.Logger JSONOutput bool JSONShorthandPayloads bool @@ -143,11 +145,7 @@ func (c *CommandContext) preprocessOptions() error { if c.Err() != nil { err = fmt.Errorf("program interrupted") } - if c.Logger != nil { - c.Logger.Error(err.Error()) - } else { - fmt.Fprintln(os.Stderr, err) - } + fmt.Fprintf(c.Options.Stderr, "Error: %v\n", err) os.Exit(1) } } @@ -260,13 +258,11 @@ func UnmarshalProtoJSONWithOptions(b []byte, m proto.Message, jsonShorthandPaylo return opts.Unmarshal(b, m) } -// Set flag values from environment file & variables. Returns a callback to log anything interesting -// since logging will not yet be initialized when this runs. -func (c *CommandContext) populateFlagsFromEnv(flags *pflag.FlagSet) (func(*slog.Logger), error) { +// Set flag values from environment file & variables. +func (c *CommandContext) populateFlagsFromEnv(flags *pflag.FlagSet) error { if flags == nil { - return func(logger *slog.Logger) {}, nil + return nil } - var logCalls []func(*slog.Logger) var flagErr error flags.VisitAll(func(flag *pflag.Flag) { // If the flag was already changed by the user, we don't overwrite @@ -289,20 +285,14 @@ func (c *CommandContext) populateFlagsFromEnv(flags *pflag.FlagSet) (func(*slog. return } if flag.Changed { - logCalls = append(logCalls, func(l *slog.Logger) { - l.Info("Env var overrode --env setting", "env_var", anns[0], "flag", flag.Name) - }) + fmt.Fprintf(c.Options.Stderr, + "Warning: env var %s overrode --env setting for flag --%s\n", anns[0], flag.Name) } flag.Changed = true } } }) - logFn := func(logger *slog.Logger) { - for _, call := range logCalls { - call(logger) - } - } - return logFn, flagErr + return flagErr } // Returns error if JSON output enabled @@ -459,8 +449,7 @@ func (c *TemporalCommand) initCommand(cctx *CommandContext) { cctx.CurrentCommand = cmd // Populate environ. We will make the error return here which will cause // usage to be printed. - logCalls, err := cctx.populateFlagsFromEnv(cmd.Flags()) - if err != nil { + if err := cctx.populateFlagsFromEnv(cmd.Flags()); err != nil { return err } @@ -472,8 +461,6 @@ func (c *TemporalCommand) initCommand(cctx *CommandContext) { res := c.preRun(cctx) - logCalls(cctx.Logger) - // Always disable color if JSON output is on (must be run after preRun so JSONOutput is set) if cctx.JSONOutput { color.NoColor = true diff --git a/internal/temporalcli/commands.operator_namespace.go b/internal/temporalcli/commands.operator_namespace.go index ebcc96cab..ca4b6c6d5 100644 --- a/internal/temporalcli/commands.operator_namespace.go +++ b/internal/temporalcli/commands.operator_namespace.go @@ -19,7 +19,7 @@ func (c *TemporalOperatorCommand) getNSFromFlagOrArg0(cctx *CommandContext, args } if len(args) > 0 { - cctx.Logger.Warn("Passing the namespace as an argument is now deprecated; please switch to using -n instead") + fmt.Fprintln(cctx.Options.Stderr, "Warning: Passing the namespace as an argument is now deprecated; please switch to using -n instead") return args[0], nil } return c.Namespace, nil diff --git a/internal/temporalcli/commands.server.go b/internal/temporalcli/commands.server.go index c484344b5..ac216423f 100644 --- a/internal/temporalcli/commands.server.go +++ b/internal/temporalcli/commands.server.go @@ -134,7 +134,6 @@ func (t *TemporalServerStartDevCommand) run(cctx *CommandContext, args []string) // Log config if requested if t.LogConfig { opts.LogConfig = func(b []byte) { - cctx.Logger.Info("Logging config") _, _ = cctx.Options.Stderr.Write(b) } } diff --git a/internal/temporalcli/commands.workflow_exec.go b/internal/temporalcli/commands.workflow_exec.go index d5ba34899..935504b96 100644 --- a/internal/temporalcli/commands.workflow_exec.go +++ b/internal/temporalcli/commands.workflow_exec.go @@ -94,7 +94,7 @@ func (c *TemporalWorkflowExecuteCommand) run(cctx *CommandContext, args []string // Log print failure and return workflow failure if workflow failed if closeEvent.EventType != enums.EVENT_TYPE_WORKFLOW_EXECUTION_COMPLETED { if err != nil { - cctx.Logger.Error("Workflow failed, and printing the output also failed", "error", err) + fmt.Fprintf(cctx.Options.Stderr, "Warning: printing workflow output failed: %v\n", err) } err = fmt.Errorf("workflow failed") } diff --git a/internal/temporalcli/commands.workflow_view.go b/internal/temporalcli/commands.workflow_view.go index 38daf936a..278fcad35 100644 --- a/internal/temporalcli/commands.workflow_view.go +++ b/internal/temporalcli/commands.workflow_view.go @@ -564,7 +564,7 @@ func (c *TemporalWorkflowResultCommand) run(cctx *CommandContext, _ []string) er // Log print failure and return workflow failure if workflow failed if closeEvent.EventType != enums.EVENT_TYPE_WORKFLOW_EXECUTION_COMPLETED { if err != nil { - cctx.Logger.Error("Workflow failed, and printing the output also failed", "error", err) + fmt.Fprintf(cctx.Options.Stderr, "Warning: printing workflow output failed: %v\n", err) } err = fmt.Errorf("workflow failed") } diff --git a/internal/temporalcli/commands.yaml b/internal/temporalcli/commands.yaml index d293b75f8..607b6b499 100644 --- a/internal/temporalcli/commands.yaml +++ b/internal/temporalcli/commands.yaml @@ -2428,7 +2428,7 @@ commands: Can be passed multiple times. - name: log-config type: bool - description: Log the server config to stderr. + description: Print the server config to stderr. - name: search-attribute type: string[] description: | diff --git a/internal/temporalcli/commands_test.go b/internal/temporalcli/commands_test.go index 2312e442e..d37c890b9 100644 --- a/internal/temporalcli/commands_test.go +++ b/internal/temporalcli/commands_test.go @@ -625,6 +625,26 @@ func TestUnknownCommandExitsNonzero(t *testing.T) { assert.Contains(t, res.Err.Error(), "unknown command") } +func TestErrorsAreNotLogMessages(t *testing.T) { + // Errors must be reported through Fail regardless of --log-level, and + // must not produce structured log output on stderr. + for _, logLevel := range []string{"never", "error", "info"} { + t.Run("log-level="+logLevel, func(t *testing.T) { + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", "not-a-valid-host:1", + "--log-level", logLevel, + ) + require.Error(t, res.Err) + + stderr := res.Stderr.String() + assert.NotContains(t, stderr, "level=", + "errors should not appear as structured log messages on stderr") + }) + } +} + func (s *SharedServerSuite) TestHiddenAliasLogFormat() { _ = s.waitActivityStarted().GetID() res := s.Execute("workflow", "list", "--log-format", "pretty", "--address", s.Address())