Fixes improper usage of log.Error(nil,..) scenarios for EventHandler

Author: camilamacedo86

Makefile

@@ -103,9 +103,8 @@ lint: lint-custom $(GOLANGCI_LINT) #HELP Run golangci linter.
 	$(GOLANGCI_LINT) run --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS)
 
 .PHONY: custom-linter-build
-LINTER_DIR := ./hack/ci/custom-linters/cmd
 custom-linter-build: # HELP Build custom linter
-	cd $(LINTER_DIR) && go build -tags '$(GO_BUILD_TAGS)' -o $(ROOT_DIR)/bin/custom-linter
+	go build -tags $(GO_BUILD_TAGS) -o ./bin/custom-linter ./hack/ci/custom-linters/cmd
 
 .PHONY: lint-custom #HELP Call custom linter for the project
 lint-custom: custom-linter-build

go.mod

@@ -26,6 +26,7 @@ require (
 	github.com/stretchr/testify v1.10.0
 	golang.org/x/exp v0.0.0-20241009180824-f66d83c29e7c
 	golang.org/x/sync v0.11.0
+	golang.org/x/tools v0.29.0
 	gopkg.in/yaml.v2 v2.4.0
 	helm.sh/helm/v3 v3.17.0
 	k8s.io/api v0.32.0
@@ -232,7 +233,6 @@ require (
 	golang.org/x/term v0.28.0 // indirect
 	golang.org/x/text v0.21.0 // indirect
 	golang.org/x/time v0.7.0 // indirect
-	golang.org/x/tools v0.29.0 // indirect
 	gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect
 	google.golang.org/genproto v0.0.0-20240903143218-8af14fe29dc1 // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1 // indirect

hack/ci/custom-linters/analyzers/setuplognilerrorcheck.go

@@ -2,7 +2,6 @@ package analyzers
 
 import (
 	"bytes"
-	"fmt"
 	"go/ast"
 	"go/format"
 	"go/token"
@@ -75,10 +74,9 @@ func runSetupLogNilErrorCheck(pass *analysis.Pass) (interface{}, error) {
 				pass.Reportf(callExpr.Pos(),
 					"Incorrect usage of 'logger.Error(nil, ...)'. The first argument must be a non-nil 'error'. "+
 						"Passing 'nil' results in silent failures, making debugging harder.\n\n"+
-						"\U0001F41B **What is wrong?**\n"+
-						fmt.Sprintf("   %s\n\n", sourceLine)+
-						"\U0001F4A1 **How to solve? Return the error, i.e.:**\n"+
-						fmt.Sprintf("   logger.Error(%s, %s, \"key\", value)\n\n", suggestedError, suggestedMessage))
+						"\U0001F41B **What is wrong?**\n   %s\n\n"+
+						"\U0001F4A1 **How to solve? Return the error, i.e.:**\n   logger.Error(%s, %s, \"key\", value)\n\n",
+					sourceLine, suggestedError, suggestedMessage)
 			}
 
 			return true

hack/ci/custom-linters/cmd/main.go

@@ -1,9 +1,10 @@
 package main
 
 import (
-	"github.com/operator-framework/operator-controller/hack/ci/custom-linters/analyzers"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/unitchecker"
+
+	"github.com/operator-framework/operator-controller/hack/ci/custom-linters/analyzers"
 )
 
 // Define the custom Linters implemented in the project

hack/ci/custom-linters/go.mod

@@ -38,6 +38,7 @@ limitations under the License.
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	cgocache "k8s.io/client-go/tools/cache"
@@ -94,8 +95,11 @@ func (e *EventHandler[object, request]) OnAdd(obj interface{}) {
 	if o, ok := obj.(object); ok {
 		c.Object = o
 	} else {
-		log.Error(nil, "OnAdd missing Object",
-			"object", obj, "type", fmt.Sprintf("%T", obj))
+		log.Error(errors.New("failed to cast object"),
+			"OnAdd missing Object",
+			"expected_type", fmt.Sprintf("%T", c.Object),
+			"received_type", fmt.Sprintf("%T", obj),
+			"object", obj)
 		return
 	}
 
@@ -118,20 +122,27 @@ func (e *EventHandler[object, request]) OnUpdate(oldObj, newObj interface{}) {
 	if o, ok := oldObj.(object); ok {
 		u.ObjectOld = o
 	} else {
-		log.Error(nil, "OnUpdate missing ObjectOld",
-			"object", oldObj, "type", fmt.Sprintf("%T", oldObj))
+		log.Error(errors.New("failed to cast old object"),
+			"OnUpdate missing ObjectOld",
+			"object", oldObj,
+			"expected_type", fmt.Sprintf("%T", u.ObjectOld),
+			"received_type", fmt.Sprintf("%T", oldObj))
 		return
 	}
 
 	// Pull Object out of the object
 	if o, ok := newObj.(object); ok {
 		u.ObjectNew = o
 	} else {
-		log.Error(nil, "OnUpdate missing ObjectNew",
-			"object", newObj, "type", fmt.Sprintf("%T", newObj))
+		log.Error(errors.New("failed to cast new object"),
+			"OnUpdate missing ObjectNew",
+			"object", newObj,
+			"expected_type", fmt.Sprintf("%T", u.ObjectNew),
+			"received_type", fmt.Sprintf("%T", newObj))
 		return
 	}
 
+	// Run predicates before proceeding
 	for _, p := range e.predicates {
 		if !p.Update(u) {
 			return
@@ -148,18 +159,25 @@ func (e *EventHandler[object, request]) OnUpdate(oldObj, newObj interface{}) {
 func (e *EventHandler[object, request]) OnDelete(obj interface{}) {
 	d := event.TypedDeleteEvent[object]{}
 
+	// Handle tombstone events (cache.DeletedFinalStateUnknown)
+	if obj == nil {
+		log.Error(errors.New("received nil object"),
+			"OnDelete received a nil object, ignoring event")
+		return
+	}
+
 	// Deal with tombstone events by pulling the object out.  Tombstone events wrap the object in a
 	// DeleteFinalStateUnknown struct, so the object needs to be pulled out.
 	// Copied from sample-controller
 	// This should never happen if we aren't missing events, which we have concluded that we are not
 	// and made decisions off of this belief.  Maybe this shouldn't be here?
-	var ok bool
-	if _, ok = obj.(client.Object); !ok {
+	if _, ok := obj.(client.Object); !ok {
 		// If the object doesn't have Metadata, assume it is a tombstone object of type DeletedFinalStateUnknown
 		tombstone, ok := obj.(cgocache.DeletedFinalStateUnknown)
 		if !ok {
-			log.Error(nil, "Error decoding objects.  Expected cache.DeletedFinalStateUnknown",
-				"type", fmt.Sprintf("%T", obj),
+			log.Error(errors.New("unexpected object type"),
+				"Error decoding objects, expected cache.DeletedFinalStateUnknown",
+				"received_type", fmt.Sprintf("%T", obj),
 				"object", obj)
 			return
 		}
@@ -175,8 +193,11 @@ func (e *EventHandler[object, request]) OnDelete(obj interface{}) {
 	if o, ok := obj.(object); ok {
 		d.Object = o
 	} else {
-		log.Error(nil, "OnDelete missing Object",
-			"object", obj, "type", fmt.Sprintf("%T", obj))
+		log.Error(errors.New("failed to cast object"),
+			"OnDelete missing Object",
+			"expected_type", fmt.Sprintf("%T", d.Object),
+			"received_type", fmt.Sprintf("%T", obj),
+			"object", obj)
 		return
 	}