diff --git a/hack/tools/logcheck/main.go b/hack/tools/logcheck/main.go index c549d7ff..e7dbe664 100644 --- a/hack/tools/logcheck/main.go +++ b/hack/tools/logcheck/main.go @@ -82,21 +82,22 @@ func checkForFunctionExpr(fun ast.Expr, args []ast.Expr, pass *analysis.Pass) { // extracting package name pName, ok := selExpr.X.(*ast.Ident) - if ok && pName.Name == "klog" && !isUnstructured((fName)) { - if fName == "InfoS" { - isKeysValid(args[1:], fun, pass, fName) - } else if fName == "ErrorS" { - isKeysValid(args[2:], fun, pass, fName) + if ok && pName.Name == "klog" { + // Matching if any unstructured logging function is used. + if !isUnstructured((fName)) { + if fName == "InfoS" { + isKeysValid(args[1:], fun, pass, fName) + } else if fName == "ErrorS" { + isKeysValid(args[2:], fun, pass, fName) + } + } else { + msg := fmt.Sprintf("unstructured logging function %q should not be used", fName) + pass.Report(analysis.Diagnostic{ + Pos: fun.Pos(), + Message: msg, + }) } - } - // Matching if package name is klog and any unstructured logging function is used. - if ok && pName.Name == "klog" && isUnstructured((fName)) { - msg := fmt.Sprintf("unstructured logging function %q should not be used", fName) - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: msg, - }) } } } @@ -125,33 +126,35 @@ func isKeysValid(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funNam if len(keyValues)%2 != 0 { pass.Report(analysis.Diagnostic{ Pos: fun.Pos(), - Message: fmt.Sprintf("Invalid Number of arguments for %s", funName), + Message: fmt.Sprintf("Additional arguments to %s should always be Key Value pairs. Please check if there is any key or value missing.", funName), }) + return } for index, arg := range keyValues { if index%2 != 0 { continue } - if lit, ok := arg.(*ast.BasicLit); ok { - if lit.Kind != token.STRING { - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: fmt.Sprintf("Invalid value type for key %v", lit.Value), - }) - continue - } - isASCII := utf8string.NewString(lit.Value).IsASCII() - if !isASCII { - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: fmt.Sprintf("Invalid value for key %v, it's must be ascii", lit.Value), - }) - } - } else { + lit, ok := arg.(*ast.BasicLit) + if !ok { + pass.Report(analysis.Diagnostic{ + Pos: fun.Pos(), + Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value", arg), + }) + continue + } + if lit.Kind != token.STRING { + pass.Report(analysis.Diagnostic{ + Pos: fun.Pos(), + Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value", lit.Value), + }) + continue + } + isASCII := utf8string.NewString(lit.Value).IsASCII() + if !isASCII { pass.Report(analysis.Diagnostic{ Pos: fun.Pos(), - Message: "Invalid value type for key", + Message: fmt.Sprintf("Key positional arguments %s are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.", lit.Value), }) } } diff --git a/hack/tools/logcheck/testdata/data.go b/hack/tools/logcheck/testdata/data.go index ec48dd8c..882a626e 100644 --- a/hack/tools/logcheck/testdata/data.go +++ b/hack/tools/logcheck/testdata/data.go @@ -44,10 +44,13 @@ func printUnstructuredLog() { func printStructuredLog() { klog.InfoS("test log") klog.ErrorS(nil, "test log") - klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod") // want `Invalid Number of arguments for InfoS` - klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod") // want `Invalid Number of arguments for ErrorS` - klog.InfoS("Starting container in a pod", "测试", "containerID") // want `Invalid value for key` - klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID") // want `Invalid value for key` - klog.InfoS("Starting container in a pod", 7, "containerID") // want `Invalid value type for key 7` - klog.ErrorS(nil, "Starting container in a pod", 7, "containerID") // want `Invalid value type for key 7` + klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod") // want `Additional arguments to InfoS should always be Key Value pairs. Please check if there is any key or value missing.` + klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod") // want `Additional arguments to ErrorS should always be Key Value pairs. Please check if there is any key or value missing.` + klog.InfoS("Starting container in a pod", "测试", "containerID") // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.` + klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID") // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.` + klog.InfoS("Starting container in a pod", 7, "containerID") // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value` + klog.ErrorS(nil, "Starting container in a pod", 7, "containerID") // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value` + klog.InfoS("Starting container in a pod", map[string]string{"test1": "value"}, "containerID") // want `Key positional arguments are expected to be inlined constant strings. ` + testKey := "a" + klog.ErrorS(nil, "Starting container in a pod", testKey, "containerID") // want `Key positional arguments are expected to be inlined constant strings. ` }