Fix by pr suggestions

kubernetes · Mar 20, 2021 · a18bc97 · a18bc97
1 parent 4e4135c
commit a18bc97
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 37 deletions.
diff --git 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
@@ -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. `
 }