Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion configs/error-log-review/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ repositories:
- name: "variables_and_functions"
description: "Pattern for variables and function calls"
regex: '(?:^|\s)(?:(?:[^)]+\.)?(?:log|logutil|logger|brlog)(?:\.(?:L\(\)|CL\([^)]*\)|FromContext\([^)]*\)|Logger\([^)]*\)|[A-Za-z]+Logger\([^)]*\))\)?)?)\.(Error|Panic|Fatal|Fatalf|Fatalln)\(\s*([^,)]+?)(?:\s*,\s*[^)]+?)?\s*\)'
- name: "go_mod_version"
description: "Pattern for go.mod go and toolchain directives"
regex: '^\s*(?:go\s+\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?|toolchain\s+go\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?)\s*$'
files:
- "go.mod"
approvers:
- "zanmato1984"
- "fixdb"
Expand All @@ -32,6 +37,11 @@ repositories:
- name: "variables_and_functions"
description: "Pattern for variables and function calls"
regex: '(?:^|\s)(?:(?:[^)]+\.)?(?:log|logutil|logger|brlog)(?:\.(?:L\(\)|CL\([^)]*\)|FromContext\([^)]*\)|Logger\([^)]*\)|[A-Za-z]+Logger\([^)]*\))\)?)?)\.(Error|Panic|Fatal|Fatalf|Fatalln)\(\s*([^,)]+?)(?:\s*,\s*[^)]+?)?\s*\)'
- name: "go_mod_version"
description: "Pattern for go.mod go and toolchain directives"
regex: '^\s*(?:go\s+\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?|toolchain\s+go\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?)\s*$'
files:
- "go.mod"
approvers:
- "niubell"

Expand All @@ -43,6 +53,11 @@ repositories:
- name: "variables_and_functions"
description: "Pattern for variables and function calls"
regex: '(?:^|\s)(?:(?:[^)]+\.)?(?:log|logutil|logger|brlog)(?:\.(?:L\(\)|CL\([^)]*\)|FromContext\([^)]*\)|Logger\([^)]*\)|[A-Za-z]+Logger\([^)]*\))\)?)?)\.(Error|Panic|Fatal|Fatalf|Fatalln)\(\s*([^,)]+?)(?:\s*,\s*[^)]+?)?\s*\)'
- name: "go_mod_version"
description: "Pattern for go.mod go and toolchain directives"
regex: '^\s*(?:go\s+\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?|toolchain\s+go\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?)\s*$'
files:
- "go.mod"
approvers:
- "Benjamin2037"
- "flowbehappy"
Expand All @@ -59,6 +74,11 @@ repositories:
regex: '(?:^|\s)(?:(?:[^)]+\.)?(?:log|logutil|logger|brlog)(?:\.(?:L\(\)|CL\([^)]*\)|FromContext\([^)]*\)|Logger\([^)]*\)|[A-Za-z]+Logger\([^)]*\))\)?)?)\.(Error|Panic|Fatal|Fatalf|Fatalln)\(\s*([^,)]+?)(?:\s*,\s*[^)]+?)?\s*\)'
excludes:
- "tests/**"
- name: "go_mod_version"
description: "Pattern for go.mod go and toolchain directives"
regex: '^\s*(?:go\s+\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?|toolchain\s+go\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?)\s*$'
files:
- "go.mod"
approvers:
- "flowbehappy"

Expand All @@ -70,6 +90,11 @@ repositories:
- name: "error_variables"
description: "Pattern for error variables"
regex: '(?:^|\s)error!\s*(\?%?\w+(?:\s*,\s*[^)]*)?)'
- name: "go_mod_version"
description: "Pattern for go.mod go and toolchain directives"
regex: '^\s*(?:go\s+\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?|toolchain\s+go\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?)\s*$'
files:
- "go.mod"
approvers:
- "zhangjinpeng87"
- "BornChanger"
Expand All @@ -86,6 +111,11 @@ repositories:
- name: "default_messages"
description: "Default pattern for other message types"
regex: '(?:^|\s)LOG_(ERROR|FATAL)\s*\(\s*[^,]+,\s*([^,)]+?)(?:\s*,\s*[^)]+)?\s*\)'
- name: "go_mod_version"
description: "Pattern for go.mod go and toolchain directives"
regex: '^\s*(?:go\s+\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?|toolchain\s+go\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?)\s*$'
files:
- "go.mod"
approvers:
- "windtalker"
- "kolafish"
Expand All @@ -105,4 +135,4 @@ settings:
# "check_and_warn" - Check and log warning, but don't fail check
mode: "check_and_fail"
# Optional hint printed when approval is required
hint: "Update error log patterns and approvers at https://github.com/PingCAP-QE/ci/blob/main/configs/error-log-review/config.yaml."
hint: "Update review patterns and approvers at https://github.com/PingCAP-QE/ci/blob/main/configs/error-log-review/config.yaml."
8 changes: 6 additions & 2 deletions tools/error-log-review/README.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
# Error Log Review

A lightweight Go program to automatically check PR diffs for error logging changes and enforce approval requirements.
A lightweight Go program to automatically check PR diffs for protected changes such as error logging edits and selected `go.mod` version upgrades, then enforce approval requirements.

## Features

- Regex-based pattern matching for different repositories
- Optional file-scoped rules, including `go.mod` `go`/`toolchain` directives
- GitHub API integration for PR diff analysis
- Approval verification through ti-chi-bot approval comments
- Configurable CI behavior (fail or warn)
Expand Down Expand Up @@ -59,7 +60,8 @@ go build -o error-log-review

The tool uses a YAML configuration file to define:

- Repository-specific log patterns to detect
- Repository-specific patterns to detect
- Optional file scopes for patterns that should only match specific files
- Required approvers for each repository
- Exclusion patterns to skip certain files/directories
- Global behavior settings
Expand All @@ -75,6 +77,8 @@ repositories:
- name: "pattern_name"
description: "Pattern description"
regex: "regular_expression"
files: # Optional: restrict the pattern to specific files
- "go.mod"
excludes: # Optional: pattern-specific exclusions
- "tests/**"
- "*_test.go"
Expand Down
31 changes: 31 additions & 0 deletions tools/error-log-review/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"os"
"path/filepath"
"regexp"
"strings"

"gopkg.in/yaml.v3"
)
Expand All @@ -13,6 +14,7 @@ type Pattern struct {
Name string `yaml:"name"`
Description string `yaml:"description"`
Regex string `yaml:"regex"`
Files []string `yaml:"files,omitempty"`
Excludes []string `yaml:"excludes,omitempty"`
compiled *regexp.Regexp
}
Expand Down Expand Up @@ -102,3 +104,32 @@ func matchesAnyPattern(filePath string, patterns []string) bool {
}
return false
}

// matchesFileScope checks if a file path is in scope for a file-limited pattern.
// Patterns without a path separator match against the file basename so "go.mod"
// applies to nested modules like "tools/foo/go.mod" as well as the repository root.
func matchesFileScope(filePath string, patterns []string) bool {
if len(patterns) == 0 {
return true
}

base := filepath.Base(filePath)
for _, pattern := range patterns {
if strings.Contains(pattern, "/") {
if matchesAnyPattern(filePath, []string{pattern}) {
return true
}
continue
}

matched, err := filepath.Match(pattern, base)
if err != nil {
continue
}
if matched {
return true
}
}

return false
}
16 changes: 13 additions & 3 deletions tools/error-log-review/config.yaml.example
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,13 @@ repositories:
description: "Pattern for variables and function calls in Go logging"
regex: '(?:^|\s)(?:(?:[^)]+\.)?(?:log|logutil|logger|brlog)(?:\.(?:L\(\)|CL\([^)]*\)|FromContext\([^)]*\)|Logger\([^)]*\)|[A-Za-z]+Logger\([^)]*\))\)?)?)\.(Error|Panic|Fatal|Fatalf|Fatalln)\(\s*([^,)]+?)(?:\s*,\s*[^)]+?)?\s*\)'
# This pattern doesn't have excludes, so it applies to all files
- name: "go_mod_version"
description: "Pattern for go.mod go and toolchain directives"
regex: '^\s*(?:go\s+\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?|toolchain\s+go\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?)\s*$'
files:
- "go.mod" # Match go.mod at the repo root or in nested modules

# List of GitHub usernames who can approve error log changes for this repository
# List of GitHub usernames who can approve protected changes for this repository
approvers:
- "maintainer1"
- "maintainer2"
Expand All @@ -47,6 +52,11 @@ repositories:
- name: "error_variables"
description: "Pattern for error variables in Rust error! macro"
regex: '(?:^|\s)error!\s*(\?%?\w+(?:\s*,\s*[^)]*)?)'
- name: "go_mod_version"
description: "Pattern for go.mod go and toolchain directives"
regex: '^\s*(?:go\s+\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?|toolchain\s+go\d+\.\d+(?:\.\d+)?(?:[A-Za-z0-9.-]+)?)\s*$'
files:
- "go.mod"

approvers:
- "rust-maintainer1"
Expand All @@ -71,5 +81,5 @@ settings:

# Optional: A hint message printed when approval is required.
# This can include pointers to where to update patterns and approvers.
# For example: "Edit configs/error-log-review/config.yaml under the corresponding repository to update patterns/approvers."
hint: "Update error log patterns/approvers at https://github.com/PingCAP-QE/ci/blob/main/configs/error-log-review/config.yaml"
# For example: "Edit configs/error-log-review/config.yaml under the corresponding repository to update review patterns/approvers."
hint: "Update review patterns/approvers at https://github.com/PingCAP-QE/ci/blob/main/configs/error-log-review/config.yaml"
22 changes: 11 additions & 11 deletions tools/error-log-review/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func main() {
// Check if this repository is configured
repo := config.GetRepository(repoName)
if repo == nil {
fmt.Printf("Repository %s is not configured for error log review - skipping\n", repoName)
fmt.Printf("Repository %s is not configured for protected change review - skipping\n", repoName)
os.Exit(0)
}

Expand All @@ -75,19 +75,19 @@ func main() {
// Initialize checker
checker := NewErrorLogChecker(config)

// Check for error log patterns
// Check for protected change patterns
matches, err := checker.CheckPRDiff(repoName, diff)
if err != nil {
log.Fatalf("Failed to check PR diff: %v", err)
}

// If no error log matches found, exit successfully
// If no protected change matches are found, exit successfully
if len(matches) == 0 {
fmt.Println("No error log changes detected - check passed")
fmt.Println("No protected changes detected - check passed")
os.Exit(0)
}

fmt.Printf("Found %d error log matches\n", len(matches))
fmt.Printf("Found %d protected change matches\n", len(matches))
for _, match := range matches {
fmt.Printf(" %s [%s]: %s\n", match.File, match.Pattern, match.Line)
}
Expand All @@ -99,25 +99,25 @@ func main() {
}

if hasApproval {
fmt.Printf("Error log changes approved by: %s\n", strings.Join(approvers, ", "))
fmt.Printf("Protected changes approved by: %s\n", strings.Join(approvers, ", "))

// Show additional repository information
requiredApprovers := checker.GetRequiredApprovers(repoName)
minApprovals := config.Settings.MinApprovals

fmt.Printf("Repository %s error log approvers: %s\n", repoName, strings.Join(requiredApprovers, ", "))
fmt.Printf("Repository %s approvers: %s\n", repoName, strings.Join(requiredApprovers, ", "))
fmt.Printf("Approval status: %d/%d required approvals received\n", len(approvers), minApprovals)

os.Exit(0)
}

// No approval found - handle based on configuration
fmt.Println("Error log changes found but not approved by repository log approvers")
fmt.Println("Protected changes found but not approved by repository approvers")

// Always show required approvers, regardless of comment settings
requiredApprovers := checker.GetRequiredApprovers(repoName)
if len(requiredApprovers) > 0 {
fmt.Printf("Repository log approvers for %s: %s\n", repoName, strings.Join(requiredApprovers, ", "))
fmt.Printf("Repository approvers for %s: %s\n", repoName, strings.Join(requiredApprovers, ", "))
}

// Print optional hint from config to guide contributors/maintainers
Expand All @@ -138,10 +138,10 @@ func main() {

// Decide CI result based on configuration
if ciMode == "check_and_fail" {
fmt.Println("Error log changes require approval - failing CI")
fmt.Println("Protected changes require approval - failing CI")
os.Exit(1)
} else {
fmt.Println("Error log changes require approval - continuing with warning")
fmt.Println("Protected changes require approval - continuing with warning")
os.Exit(0)
}
}
4 changes: 4 additions & 0 deletions tools/error-log-review/matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ func (clc *ErrorLogChecker) CheckPRDiff(repoName, diff string) ([]Match, error)

// Check against all patterns for this repository
for _, pattern := range repo.Patterns {
if !matchesFileScope(currentFile, pattern.Files) {
continue
}

// Check if current file is excluded for this specific pattern
if matchesAnyPattern(currentFile, pattern.Excludes) {
continue
Expand Down
Loading