CLOUDP-239791: Replace log.Fatal calls in main.go with run() error pattern#941
CLOUDP-239791: Replace log.Fatal calls in main.go with run() error pattern#941m1kola wants to merge 1 commit intomongodb:masterfrom
Conversation
…ttern Extract operator startup logic into a run() function that returns an error, so deferred cleanup (tracer shutdown, span end) runs on failure instead of being bypassed by os.Exit via log.Fatal.
m1kola
added
the
skip-changelog
| log.Error(err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
q: why not making this a fatal? I would also wrap the error with some info.
| log.Error(err) | |
| os.Exit(1) | |
| log.Fatalf("Fatal error: %v", err) |
There was a problem hiding this comment.
I normally prefer explicit over implicit. And log.Fatalf has an implicit exit.
But I don't mind changing it to log.Fatalf, if you prefer.
| mgr, err := ctrl.NewManager(cfg, managerOptions) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| return err |
There was a problem hiding this comment.
nit: it is usually best to wrap the error context. E.g:
| return err | |
| return fmt.Errorf("failed to create manager: %w", err) |
There was a problem hiding this comment.
This is startup code in main, not a library/package. The errors from ctrl.NewManager, apiv1.AddToScheme, etc. already return descriptive messages. Wrapping them adds boilerplate without giving the caller anything useful. And there's no caller that needs to errors.Is/errors.As these, and the log output in main will print the original message anyway.
The point of the ticket was to fix the os.Exit bypass, not to improve error messages.
2 participants
Summary
Extract operator startup logic into a
run()function that returns anerror, so deferred cleanup (tracer shutdown, span end) runs on failure instead of being bypassed byos.Exitvialog.Fatal.Proof of Work
CI must be green
Checklist
skip-changeloglabel if not needed