(WIP) 🐛 Fix: Prevent nil errors in setupLog.Error to ensure proper logging and add sanity check

[camilamacedo86]

Description

Fix: Prevent nil errors in setupLog.Error to ensure proper logging
Closes; #1566
Closes: #1556

Reviewer Checklist

• API Go Documentation
• Tests: Unit Tests (and E2E Tests, if appropriate)
• Comprehensive Commit Messages
• Links to related GitHub Issue(s)

[netlify]

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	a66fb03
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6783b1181f70ed0008b68d0b
😎 Deploy Preview	https://deploy-preview-1599--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[joelanford]

Oh I like this!

Nit: I'd probably name this hack/ci/extra_linters.sh
Also nit: Is there a way to define the rules and messages such that they are setup as a list of pairs, and then we go ahead and write a loop to iterate and run each of them?

Lastly, I kinda wonder if there's already a generic regex-based linter built into golangci-lint that we could add custom rules to? 🤔

[camilamacedo86]

Good points, I will try to check them out.

[joelanford]

We may also want to look into writing custom analyzers in Go using golang.org/x/tools/go/analysis. As is, if we have a variable name for a logger that is not setupLog, we'll miss it.

I assume an analyzer could find all logr.Logger.Error() method usage where the first parameter is nil.

[joelanford]

But now I'm getting really picky :)

[camilamacedo86]

I think the idea of the custom analyse is nice
I will try it out.

[camilamacedo86]

Not sure if it will be so easier.
I need to install locally

  574  brew install pkg-config
  576  brew install gpgme

Then, I could run

$ go vet -vettool=./bin/custom-linter ./...
# github.com/operator-framework/operator-controller/catalogd/cmd/catalogd
# [github.com/operator-framework/operator-controller/catalogd/cmd/catalogd]
catalogd/cmd/catalogd/main.go:146:3: Avoid using 'setupLog.Error(nil, ...)'. Instead, use 'errors.New()' or 'fmt.Errorf()' to ensure logs are created. Using 'nil' for errors can result in silent failures, making bugs harder to detect.
catalogd/cmd/catalogd/main.go:151:3: Avoid using 'setupLog.Error(nil, ...)'. Instead, use 'errors.New()' or 'fmt.Errorf()' to ensure logs are created. Using 'nil' for errors can result in silent failures, making bugs harder to detect.
# github.com/operator-framework/operator-controller/cmd/operator-controller
# [github.com/operator-framework/operator-controller/cmd/operator-controller]
cmd/operator-controller/main.go:131:3: Avoid using 'setupLog.Error(nil, ...)'. Instead, use 'errors.New()' or 'fmt.Errorf()' to ensure logs are created. Using 'nil' for errors can result in silent failures, making bugs harder to detect.
cmd/operator-controller/main.go:136:3: Avoid using 'setupLog.Error(nil, ...)'. Instead, use 'errors.New()' or 'fmt.Errorf()' to ensure logs are created. Using 'nil' for errors can result in silent failures, making bugs harder to detect.

But with golang-ci custom linters did not worked
And also, when I call via make file, that fails.
Seems that I need to pass some config

Then, we endup with configs for mac/linux
Maybe better keep it KISS

####################################################
# The purpose of this script is allow us to do extra
# sanity checks to ensure the quality.
####################################################

#!/bin/bash

set -e

echo_error() {
  echo "[Sanity Check Fail]: $1"
}

# Criteria 1: Do not use setupLog.Error(nil, ...)
CRITERIA1_RULE="setupLog\.Error\(nil,"
CRITERIA1_MSG="Avoid using 'setupLog.Error(nil, ...)'. Instead, use 'errors.New()' or 'fmt.Errorf()' \
to ensure logs are created. Using 'nil' for errors can result in silent failures, making bugs harder to detect."

if grep -r --include="*.go" "$CRITERIA1_RULE" .; then
  echo_error "$CRITERIA1_MSG"
  exit 1
fi

echo "Sanity checks passed with success."
exit 0
####################################################
# The purpose of this script is allow us to do extra
# sanity checks to ensure the quality.
####################################################

#!/bin/bash

set -e

echo_error() {
  echo "[Sanity Check Fail]: $1"
}

# Criteria 1: Do not use setupLog.Error(nil, ...)
CRITERIA1_RULE="setupLog\.Error\(nil,"
CRITERIA1_MSG="Avoid using 'setupLog.Error(nil, ...)'. Instead, use 'errors.New()' or 'fmt.Errorf()' \
to ensure logs are created. Using 'nil' for errors can result in silent failures, making bugs harder to detect."

if grep -r --include="*.go" "$CRITERIA1_RULE" .; then
  echo_error "$CRITERIA1_MSG"
  exit 1
fi

echo "Sanity checks passed with success."
exit 0
####################################################
# The purpose of this script is allow us to do extra
# sanity checks to ensure the quality.
####################################################

#!/bin/bash

set -e

echo_error() {
  echo "[Sanity Check Fail]: $1"
}

# Criteria 1: Do not use setupLog.Error(nil, ...)
CRITERIA1_RULE="setupLog\.Error\(nil,"
CRITERIA1_MSG="Avoid using 'setupLog.Error(nil, ...)'. Instead, use 'errors.New()' or 'fmt.Errorf()' \
to ensure logs are created. Using 'nil' for errors can result in silent failures, making bugs harder to detect."

if grep -r --include="*.go" "$CRITERIA1_RULE" .; then
  echo_error "$CRITERIA1_MSG"
  exit 1
fi

echo "Sanity checks passed with success."
exit 0

for now.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 66.68%. Comparing base (75bb73e) to head (a66fb03).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
catalogd/cmd/catalogd/main.go	0.00%	2 Missing ⚠️
cmd/operator-controller/main.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1599   +/-   ##
=======================================
  Coverage   66.68%   66.68%           
=======================================
  Files          57       57           
  Lines        4584     4584           
=======================================
  Hits         3057     3057           
  Misses       1302     1302           
  Partials      225      225           

Flag	Coverage Δ
e2e	52.72% <0.00%> (ø)
unit	53.62% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joelanford]

The check should:

1. Look for all instances of logr.Logger (not just setupLog)
2. Of those, find when Error is called.
3. Of those, find when the first argument is nil.

That way, we'll find all cases where this is an issue, and we'll protect ourselves in the case that we decide to change the name of setupLog.

[joelanford]

Most likely, we should just say "pass a non-nil error". If we suggest fmt.Errorf(), that may incorrect lead to a contributor burying useful structured log context inside the error message, when what they should do is:

l.Error(errors.New("could not do thing"), "thing", foo, "otherThing", bar)

[joelanford]

Nit: use errors.New since we aren't using the format string capabilities of fmt.Errorf.

[joelanford]

golangci-lint should already be running go vet (assuming the govet linter is enabled). So I think we get this part automatically if we add the directory where we build the linter binary to the path for the golangci-lint run command.