-
Notifications
You must be signed in to change notification settings - Fork 157
Replace os.Exit in binoculars library with error return #4779
Copy link
Copy link
Open
Labels
component/otherA general catch-allA general catch-allgood first issueGood for newcomersGood for newcomerslanguage/goPull requests that update Go codePull requests that update Go code
Description
Is your feature request related to a problem? Please describe.
StartUp() in internal/binoculars/server.go calls os.Exit(-1) on initialization errors (lines 28 and 34) instead of returning an error. This means you can't unit test the function (the test process would just die), and it prevents the caller from doing any cleanup.
// Current code (internal/binoculars/server.go)
func StartUp(config *configuration.BinocularsConfig) (func(), *sync.WaitGroup) {
// ...
kubernetesClientProvider, err := cluster.NewKubernetesClientProvider(...)
if err != nil {
log.Errorf("Failed to connect to kubernetes because %s", err)
os.Exit(-1)
}
authServices, err := auth.ConfigureAuth(config.Auth)
if err != nil {
log.Errorf("Failed to create auth services %s", err)
os.Exit(-1)
}
// ...
}In Go, library code (anything under internal/) should return errors and let the caller in cmd/ decide what to do. Only main() should call os.Exit or log.Fatal.
Describe the solution you'd like
- Change
StartUp()to return an error:func StartUp(config *configuration.BinocularsConfig) (func(), *sync.WaitGroup, error) - Replace the
os.Exit(-1)calls with wrapped error returns - Update the caller in
cmd/binoculars/main.go:74to handle the error (log.Fatalis fine there)
Files to change
internal/binoculars/server.go- change signature, return errorscmd/binoculars/main.go- handle the new error return
There's only one caller, so this is a small change.
Acceptance criteria
- No
os.Exitininternal/binoculars/server.go StartUp()returnserroras its last return valuecmd/binoculars/main.gohandles the errorgo build ./cmd/binoculars/...succeedsgolangci-lint run ./internal/binoculars/... ./cmd/binoculars/...passes
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
component/otherA general catch-allA general catch-allgood first issueGood for newcomersGood for newcomerslanguage/goPull requests that update Go codePull requests that update Go code