infobloxopen/infoblox-go-client

A module like this should never log an error, never log anything to stdout, and should always return an error if one exists

bmayfi3ld opened this issue · 0 comments

A module should never log an error. It should always be returned to the user to decide what to do with the error (ignore or move on). When it is logged to stdout randomly by underlying packages the caller of the api can't really control or even know what is going on. This is so established, I couldn't even find an article that talks about doing it, they all just assume you are already doing it because it is so basic to the language.

https://golang.org/doc/effective_go.html#errors
https://medium.com/@hussachai/error-handling-in-go-a-quick-opinionated-guide-9199dd7c7f76
https://blog.golang.org/error-handling-and-go
https://medium.com/rungo/error-handling-in-go-f0125de052f0
https://blog.logrocket.com/error-handling-in-golang/

All of these are talking about different techniques around error handling, but all assume any non-main function is returning error for the higher level functions to manage.

I see in this codebase has a large number of areas where the log package is used to print errors, sometimes without the underlying package even returning the error.

In NewTransportConfig
if err != nil { log.Printf("Cannot load certificate file '%s'", sslVerify) return } if !caPool.AppendCertsFromPEM(cert) { err = fmt.Errorf("Cannot append certificate from file '%s'", sslVerify) return }

Error is just printed in the first block and the second block creates an error but does not return it?

In getHTTPResponseError
msg := fmt.Sprintf("WAPI request error: %d('%s')\nContents:\n%s\n", resp.StatusCode, resp.Status, content) log.Printf(msg) return errors.New(msg)

This at least returns the error but it still should not print to stdout.

if err != nil { log.Fatal(err) }
Here we kill the program because of the error. What if I wanted there to be an error here? what if I was checking for something to not work?

if err != nil { log.Printf("Http Reponse ioutil.ReadAll() Error: '%s'", err) return }

Here we at least return the error but still it should not be printed out.

There are many more examples but I think this is enough for the post.