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

Jump to bottom

[VC-35738] Remove os.Exit from the logs module #611

Merged
merged 1 commit into from
Nov 12, 2024
Merged

[VC-35738] Remove os.Exit from the logs module #611

Show file tree
Hide file tree
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
4 changes: 2 additions & 2 deletions cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Returning the error here means that the error can be logged in the same format as all the other errors that are returned by the command.

// SilenceErrors and SilenceUsage prevents this command or any sub-command
// from printing arbitrary text to stderr.
Expand Down
7 changes: 3 additions & 4 deletions pkg/logs/logs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"log"
"log/slog"
"os"
"strings"

"github.com/spf13/pflag"
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down
54 changes: 26 additions & 28 deletions pkg/logs/logs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"bytes"
"context"
"errors"
"fmt"
"io"
"log"
"log/slog"
"os"
Expand Down Expand Up @@ -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)
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are intended to make the logs test fixture parse the flags and behave the same way as Cobra.

  • --help / -h results in stdout and exit code 0
  • Unrecognized flags result in an error which is logged to stderr with exit code 1

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the extra context. I think it would be worth adding a comment above to keep that in mind in the future. I'm already imagining myself in a year: why do we do this weird dance with the exit codes in the test code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll merge this now and add the comment in one of the followup PRS.


log.Print("log Print")
Expand All @@ -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)
Expand All @@ -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
`,
},
{
Expand Down Expand Up @@ -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)
Expand Down