Remove os.Exit from the logs module and make the logs test output more realistic (#611)

Signed-off-by: Richard Wall <[email protected]>

Author: wallrj

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.

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 {

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,38 +92,24 @@ 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
 `,
 		},
 		{
 			name:        "logging-format-unrecognized",
 			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)