Skip to content

Commit 9c6c0ac

Browse files
camilamacedo86camilamacedo86
committed
Improve fix makefile helpers and custom linter code
1 parent c0bae6e commit 9c6c0ac

File tree

8 files changed

+101
-22
lines changed

8 files changed

+101
-22
lines changed

‎Makefile

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,11 +103,11 @@ lint: lint-custom $(GOLANGCI_LINT) #HELP Run golangci linter.
103103
$(GOLANGCI_LINT) run --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS)
104104

105105
.PHONY: custom-linter-build
106-
custom-linter-build: # HELP Build custom linter
106+
custom-linter-build: #HELP Build custom linter
107107
go build -tags $(GO_BUILD_TAGS) -o ./bin/custom-linter ./hack/ci/custom-linters/cmd
108108

109-
.PHONY: lint-custom #HELP Call custom linter for the project
110-
lint-custom: custom-linter-build
109+
.PHONY: lint-custom
110+
lint-custom: custom-linter-build #HELP Call custom linter for the project
111111
go vet -tags=$(GO_BUILD_TAGS) -vettool=./bin/custom-linter ./...
112112

113113
.PHONY: tidy

‎go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ require (
227227
go.opentelemetry.io/otel/trace v1.33.0 // indirect
228228
go.opentelemetry.io/proto/otlp v1.3.1 // indirect
229229
golang.org/x/crypto v0.32.0 // indirect
230+
golang.org/x/mod v0.22.0 // indirect
230231
golang.org/x/net v0.34.0 // indirect
231232
golang.org/x/oauth2 v0.25.0 // indirect
232233
golang.org/x/sys v0.29.0 // indirect

‎hack/ci/custom-linters/analyzers/analyzers_test.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package analyzers
2+
3+
import (
4+
"testing"
5+
6+
"golang.org/x/tools/go/analysis/analysistest"
7+
)
8+
9+
func TestSetupLogErrorCheck(t *testing.T) {
10+
testdata := analysistest.TestData()
11+
analysistest.Run(t, testdata, SetupLogErrorCheck)
12+
}

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

Lines changed: 51 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,13 @@ import (
1010
"golang.org/x/tools/go/analysis"
1111
)
1212

13-
var SetupLogNilErrorCheck = &analysis.Analyzer{
14-
Name: "setuplognilerrorcheck",
13+
var SetupLogErrorCheck = &analysis.Analyzer{
14+
Name: "setuplogerrorcheck",
1515
Doc: "Detects improper usage of logger.Error() calls, ensuring the first argument is a non-nil error.",
16-
Run: runSetupLogNilErrorCheck,
16+
Run: runSetupLogErrorCheck,
1717
}
1818

19-
func runSetupLogNilErrorCheck(pass *analysis.Pass) (interface{}, error) {
19+
func runSetupLogErrorCheck(pass *analysis.Pass) (interface{}, error) {
2020
for _, f := range pass.Files {
2121
ast.Inspect(f, func(n ast.Node) bool {
2222
callExpr, ok := n.(*ast.CallExpr)
@@ -51,32 +51,65 @@ func runSetupLogNilErrorCheck(pass *analysis.Pass) (interface{}, error) {
5151
return true
5252
}
5353

54-
// Check if the first argument of the error log is nil
55-
firstArg, ok := callExpr.Args[0].(*ast.Ident)
56-
if !ok || firstArg.Name != "nil" {
54+
// Get the actual source code line where the issue occurs
55+
var srcBuffer bytes.Buffer
56+
if err := format.Node(&srcBuffer, pass.Fset, callExpr); err != nil {
5757
return true
5858
}
59+
sourceLine := srcBuffer.String()
5960

60-
// Extract error message for the suggestion
61-
suggestedError := "errors.New(\"kind error (i.e. configuration error)\")"
62-
suggestedMessage := "\"error message describing the failed operation\""
61+
// Check if the first argument of the error log is nil
62+
firstArg, ok := callExpr.Args[0].(*ast.Ident)
63+
if ok && firstArg.Name == "nil" {
64+
suggestedError := "errors.New(\"kind error (i.e. configuration error)\")"
65+
suggestedMessage := "\"error message describing the failed operation\""
6366

64-
if len(callExpr.Args) > 1 {
65-
if msgArg, ok := callExpr.Args[1].(*ast.BasicLit); ok && msgArg.Kind == token.STRING {
66-
suggestedMessage = msgArg.Value
67+
if len(callExpr.Args) > 1 {
68+
if msgArg, ok := callExpr.Args[1].(*ast.BasicLit); ok && msgArg.Kind == token.STRING {
69+
suggestedMessage = msgArg.Value
70+
}
6771
}
68-
}
6972

70-
// Get the actual source code line where the issue occurs
71-
var srcBuffer bytes.Buffer
72-
if err := format.Node(&srcBuffer, pass.Fset, callExpr); err == nil {
73-
sourceLine := srcBuffer.String()
7473
pass.Reportf(callExpr.Pos(),
7574
"Incorrect usage of 'logger.Error(nil, ...)'. The first argument must be a non-nil 'error'. "+
7675
"Passing 'nil' results in silent failures, making debugging harder.\n\n"+
7776
"\U0001F41B **What is wrong?**\n %s\n\n"+
7877
"\U0001F4A1 **How to solve? Return the error, i.e.:**\n logger.Error(%s, %s, \"key\", value)\n\n",
7978
sourceLine, suggestedError, suggestedMessage)
79+
return true
80+
}
81+
82+
// Ensure at least two arguments exist (error + message)
83+
if len(callExpr.Args) < 2 {
84+
pass.Reportf(callExpr.Pos(),
85+
"Incorrect usage of 'logger.Error(error, ...)'. Expected at least an error and a message string.\n\n"+
86+
"\U0001F41B **What is wrong?**\n %s\n\n"+
87+
"\U0001F4A1 **How to solve?**\n Provide a message, e.g. logger.Error(err, \"descriptive message\")\n\n",
88+
sourceLine)
89+
return true
90+
}
91+
92+
// Ensure key-value pairs (if any) are valid
93+
if (len(callExpr.Args)-2)%2 != 0 {
94+
pass.Reportf(callExpr.Pos(),
95+
"Incorrect usage of 'logger.Error(error, \"msg\", ...)'. Key-value pairs must be provided after the message, but an odd number of arguments was found.\n\n"+
96+
"\U0001F41B **What is wrong?**\n %s\n\n"+
97+
"\U0001F4A1 **How to solve?**\n Ensure all key-value pairs are complete, e.g. logger.Error(err, \"msg\", \"key\", value, \"key2\", value2)\n\n",
98+
sourceLine)
99+
return true
100+
}
101+
102+
for i := 2; i < len(callExpr.Args); i += 2 {
103+
keyArg := callExpr.Args[i]
104+
keyType := pass.TypesInfo.TypeOf(keyArg)
105+
if keyType == nil || keyType.String() != "string" {
106+
pass.Reportf(callExpr.Pos(),
107+
"Incorrect usage of 'logger.Error(error, \"msg\", key, value)'. Keys in key-value pairs must be strings, but got: %s.\n\n"+
108+
"\U0001F41B **What is wrong?**\n %s\n\n"+
109+
"\U0001F4A1 **How to solve?**\n Ensure keys are strings, e.g. logger.Error(err, \"msg\", \"key\", value)\n\n",
110+
keyType, sourceLine)
111+
return true
112+
}
80113
}
81114

82115
return true

‎hack/ci/custom-linters/analyzers/testdata/go.mod

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
module testdata
2+
3+
go 1.23.4
4+
5+
require github.com/go-logr/logr v1.4.2

‎hack/ci/custom-linters/analyzers/testdata/go.sum

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY=
2+
github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=

‎hack/ci/custom-linters/analyzers/testdata/main.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package main
2+
3+
import (
4+
"github.com/go-logr/logr"
5+
)
6+
7+
func testLogger() {
8+
var logger logr.Logger
9+
var err error
10+
var value int
11+
12+
// Case 1: Nil error - Ensures the first argument cannot be nil.
13+
logger.Error(nil, "message") // want ".*results in silent failures, making debugging harder.*"
14+
15+
// Case 2: Odd number of key-value arguments - Ensures key-value pairs are complete.
16+
logger.Error(err, "message", "key1") // want ".*Key-value pairs must be provided after the message, but an odd number of arguments was found.*"
17+
18+
// Case 3: Key in key-value pair is not a string - Ensures keys in key-value pairs are strings.
19+
logger.Error(err, "message", 123, value) // want ".*Ensure keys are strings.*"
20+
21+
// Case 4: Values are passed without corresponding keys - Ensures key-value arguments are structured properly.
22+
logger.Error(err, "message", value, "key2", value) // want ".*Key-value pairs must be provided after the message, but an odd number of arguments was found.*"
23+
24+
// Case 5: Correct Usage - Should not trigger any warnings.
25+
logger.Error(err, "message", "key1", value, "key2", "value")
26+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import (
99

1010
// Define the custom Linters implemented in the project
1111
var customLinters = []*analysis.Analyzer{
12-
analyzers.SetupLogNilErrorCheck,
12+
analyzers.SetupLogErrorCheck,
1313
}
1414

1515
func main() {

0 commit comments

Comments
 (0)