From 3d4876b5a201d1ee6970811cc18605157fa5b420 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Thu, 7 Nov 2024 13:37:34 +0000 Subject: [PATCH] Remove os.Exit from the logs module and make the logs test output more realistic Signed-off-by: Richard Wall --- cmd/root.go | 4 ++-- pkg/logs/logs.go | 7 +++--- pkg/logs/logs_test.go | 54 +++++++++++++++++++++---------------------- 3 files changed, 31 insertions(+), 34 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index fe9e5989..e2d49ba3 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -20,8 +20,8 @@ var rootCmd = &cobra.Command{ configuration checks using Open Policy Agent (OPA). Preflight checks are bundled into Packages`, - PersistentPreRun: func(cmd *cobra.Command, args []string) { - logs.Initialize() + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return logs.Initialize() }, // SilenceErrors and SilenceUsage prevents this command or any sub-command // from printing arbitrary text to stderr. diff --git a/pkg/logs/logs.go b/pkg/logs/logs.go index b16c62e1..b8ffd71f 100644 --- a/pkg/logs/logs.go +++ b/pkg/logs/logs.go @@ -5,7 +5,6 @@ import ( "fmt" "log" "log/slog" - "os" "strings" "github.com/spf13/pflag" @@ -108,13 +107,12 @@ func AddFlags(fs *pflag.FlagSet) { // Initialize uses k8s.io/component-base/logs, to configure the following global // loggers: log, slog, and klog. All are configured to write in the same format. -func Initialize() { +func Initialize() error { // This configures the global logger in klog *and* slog, if compiled with Go // >= 1.21. logs.InitLogs() if err := logsapi.ValidateAndApply(configuration, features); err != nil { - fmt.Fprintf(os.Stderr, "Error in logging configuration: %v\n", err) - os.Exit(2) + return fmt.Errorf("Error in logging configuration: %s", err) } // Thanks to logs.InitLogs, slog.Default now uses klog as its backend. Thus, @@ -133,6 +131,7 @@ func Initialize() { // to the global log logger. It can be removed when this is fixed upstream // in vcert: https://github.com/Venafi/vcert/pull/512 vcertLog.SetPrefix("") + return nil } type LogToSlogWriter struct { diff --git a/pkg/logs/logs_test.go b/pkg/logs/logs_test.go index 58a74725..e3869df6 100644 --- a/pkg/logs/logs_test.go +++ b/pkg/logs/logs_test.go @@ -4,6 +4,8 @@ import ( "bytes" "context" "errors" + "fmt" + "io" "log" "log/slog" "os" @@ -33,10 +35,25 @@ import ( func TestLogs(t *testing.T) { if flags, found := os.LookupEnv("GO_CHILD_FLAG"); found { if _, found := os.LookupEnv("GO_CHILD_SKIP_INITIALIZE"); !found { - fs := pflag.NewFlagSet("test-logs", pflag.ExitOnError) + fs := pflag.NewFlagSet("test-logs", pflag.ContinueOnError) + fs.SetOutput(io.Discard) logs.AddFlags(fs) - fs.Parse(strings.Split(flags, " ")) - logs.Initialize() + if err := fs.Parse(strings.Split(flags, " ")); err != nil { + exitCode := 0 + if errors.Is(err, pflag.ErrHelp) { + fmt.Fprint(os.Stdout, fs.FlagUsages()) + os.Exit(exitCode) + } else { + exitCode := 1 + klog.ErrorS(err, "Exiting due to error", "exit-code", exitCode) + klog.FlushAndExit(time.Second, exitCode) + } + } + if err := logs.Initialize(); err != nil { + exitCode := 1 + klog.ErrorS(err, "Exiting due to error", "exit-code", exitCode) + klog.FlushAndExit(time.Second, exitCode) + } } log.Print("log Print") @@ -63,14 +80,9 @@ func TestLogs(t *testing.T) { expectStderr string }{ { - name: "help", - flags: "-h", - expectError: true, + name: "help", + flags: "-h", expectStdout: ` -pflag: help requested -`, - expectStderr: ` -Usage of test-logs: -v, --log-level Level number for the log level verbosity --logging-format string Sets the log format. Permitted formats: "json", "text". (default "text") --vmodule pattern=N,... comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) @@ -80,30 +92,16 @@ Usage of test-logs: name: "unrecognized-flag", flags: "--foo", expectError: true, - expectStdout: ` -unknown flag: --foo -`, expectStderr: ` -unknown flag: --foo -Usage of test-logs: - -v, --log-level Level number for the log level verbosity - --logging-format string Sets the log format. Permitted formats: "json", "text". (default "text") - --vmodule pattern=N,... comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) +E0000 00:00:00.000000 00000 logs_test.go:000] "Exiting due to error" err="unknown flag: --foo" exit-code=1 `, }, { name: "v-long-form-not-available", flags: "--v=3", expectError: true, - expectStdout: ` -unknown flag: --v -`, expectStderr: ` -unknown flag: --v -Usage of test-logs: - -v, --log-level Level number for the log level verbosity - --logging-format string Sets the log format. Permitted formats: "json", "text". (default "text") - --vmodule pattern=N,... comma-separated list of pattern=N settings for file-filtered logging (only works for text log format) +E0000 00:00:00.000000 00000 logs_test.go:000] "Exiting due to error" err="unknown flag: --v" exit-code=1 `, }, { @@ -111,7 +109,7 @@ Usage of test-logs: flags: "--logging-format=foo", expectError: true, expectStderr: ` -Error in logging configuration: format: Invalid value: "foo": Unsupported log format +E0000 00:00:00.000000 00000 logs_test.go:000] "Exiting due to error" err="Error in logging configuration: format: Invalid value: \"foo\": Unsupported log format" exit-code=1 `, }, { @@ -297,7 +295,7 @@ E0000 00:00:00.000000 00000 logs_test.go:000] "Contextual error" err="fake-err if test.expectError { var target *exec.ExitError require.ErrorAs(t, err, &target) - require.Equal(t, 2, target.ExitCode(), "Flag parsing failures should always result in exit code 2") + require.Equal(t, 1, target.ExitCode(), "Flag parsing failures should always result in exit code 1") t.Logf("ERROR: %v", err) } else { require.NoError(t, err)