diff --git a/configs/error-log-review/config.yaml b/configs/error-log-review/config.yaml index 6b24384d8..0514591ce 100644 --- a/configs/error-log-review/config.yaml +++ b/configs/error-log-review/config.yaml @@ -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" @@ -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" @@ -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" @@ -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" @@ -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" @@ -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" @@ -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." diff --git a/tools/error-log-review/README.md b/tools/error-log-review/README.md index e3bf5caec..2b79e8d9f 100644 --- a/tools/error-log-review/README.md +++ b/tools/error-log-review/README.md @@ -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) @@ -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 @@ -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" diff --git a/tools/error-log-review/config.go b/tools/error-log-review/config.go index 89a12118c..cf541a805 100644 --- a/tools/error-log-review/config.go +++ b/tools/error-log-review/config.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "regexp" + "strings" "gopkg.in/yaml.v3" ) @@ -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 } @@ -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 +} diff --git a/tools/error-log-review/config.yaml.example b/tools/error-log-review/config.yaml.example index 5416ddd82..8e79aa36b 100644 --- a/tools/error-log-review/config.yaml.example +++ b/tools/error-log-review/config.yaml.example @@ -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" @@ -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" @@ -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" diff --git a/tools/error-log-review/main.go b/tools/error-log-review/main.go index 48aa4e6c0..1712a10e2 100644 --- a/tools/error-log-review/main.go +++ b/tools/error-log-review/main.go @@ -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) } @@ -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) } @@ -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 @@ -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) } } diff --git a/tools/error-log-review/matcher.go b/tools/error-log-review/matcher.go index 2df2269dc..73b5a2c07 100644 --- a/tools/error-log-review/matcher.go +++ b/tools/error-log-review/matcher.go @@ -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 diff --git a/tools/error-log-review/matcher_test.go b/tools/error-log-review/matcher_test.go index 6134dc011..168c7d452 100644 --- a/tools/error-log-review/matcher_test.go +++ b/tools/error-log-review/matcher_test.go @@ -1,6 +1,8 @@ package main import ( + "os" + "path/filepath" "regexp" "testing" ) @@ -149,6 +151,178 @@ index 1234567..abcdefg 100644 } } +func TestLoadConfigWithFileScopedPattern(t *testing.T) { + configData := `repositories: + - name: "pingcap/tidb" + patterns: + - 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: + - "approver" +settings: + min_approvals: 1 + require_repo_specific_approvers: true + check_behavior: + mode: "check_and_fail" +` + + configPath := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(configPath, []byte(configData), 0o600); err != nil { + t.Fatalf("failed to write config fixture: %v", err) + } + + config, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig failed: %v", err) + } + + repo := config.GetRepository("pingcap/tidb") + if repo == nil { + t.Fatal("expected repository pingcap/tidb to be present") + } + + if len(repo.Patterns) != 1 { + t.Fatalf("expected 1 pattern, got %d", len(repo.Patterns)) + } + + if len(repo.Patterns[0].Files) != 1 || repo.Patterns[0].Files[0] != "go.mod" { + t.Fatalf("expected file scope [go.mod], got %v", repo.Patterns[0].Files) + } + + if repo.Patterns[0].compiled == nil { + t.Fatal("expected regex to be compiled") + } +} + +func TestLoadSharedConfigIncludesGoModReview(t *testing.T) { + configPath := filepath.Join("..", "..", "configs", "error-log-review", "config.yaml") + + config, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig failed for shared config: %v", err) + } + + for _, repo := range config.Repositories { + found := false + for _, pattern := range repo.Patterns { + if pattern.Name != "go_mod_version" { + continue + } + + found = true + if !matchesFileScope("go.mod", pattern.Files) { + t.Errorf("repository %s go_mod_version pattern should match root go.mod", repo.Name) + } + if !matchesFileScope("nested/module/go.mod", pattern.Files) { + t.Errorf("repository %s go_mod_version pattern should match nested go.mod", repo.Name) + } + } + + if !found { + t.Errorf("repository %s is missing go_mod_version pattern", repo.Name) + } + } +} + +func TestCheckPRDiffWithFileScopedPatterns(t *testing.T) { + config := &Config{ + Repositories: []Repository{ + { + Name: "pingcap/tidb", + Patterns: []Pattern{ + { + 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: []string{"go.mod"}, + }, + { + Name: "error_pattern", + Description: "Pattern for error log changes", + Regex: `log\.Error\(".*"\)`, + }, + }, + }, + }, + } + + for i := range config.Repositories { + for j := range config.Repositories[i].Patterns { + pattern := &config.Repositories[i].Patterns[j] + compiled, _ := regexp.Compile(pattern.Regex) + pattern.compiled = compiled + } + } + + checker := NewErrorLogChecker(config) + + diff := `diff --git a/go.mod b/go.mod +index 1234567..abcdefg 100644 +--- a/go.mod ++++ b/go.mod +@@ -1,2 +1,4 @@ ++go 1.24.1 ++toolchain go1.24.2 ++toolchain local +diff --git a/components/worker/go.mod b/components/worker/go.mod +index 1234567..abcdefg 100644 +--- a/components/worker/go.mod ++++ b/components/worker/go.mod +@@ -1,1 +1,2 @@ ++go 1.23.7 +diff --git a/docs/upgrade.md b/docs/upgrade.md +index 1234567..abcdefg 100644 +--- a/docs/upgrade.md ++++ b/docs/upgrade.md +@@ -10,0 +11,1 @@ ++go 1.25.0 +diff --git a/main.go b/main.go +index 1234567..abcdefg 100644 +--- a/main.go ++++ b/main.go +@@ -10,0 +11,1 @@ ++ log.Error("production error") +` + + matches, err := checker.CheckPRDiff("pingcap/tidb", diff) + if err != nil { + t.Fatalf("CheckPRDiff failed: %v", err) + } + + if len(matches) != 4 { + t.Fatalf("expected 4 matches, got %d: %+v", len(matches), matches) + } + + var goModMatches, logMatches int + for _, match := range matches { + switch match.Pattern { + case "go_mod_version": + goModMatches++ + if filepath.Base(match.File) != "go.mod" { + t.Errorf("expected go.mod scoped match, got %s", match.File) + } + case "error_pattern": + logMatches++ + if match.File != "main.go" { + t.Errorf("expected error pattern to match main.go, got %s", match.File) + } + default: + t.Errorf("unexpected pattern %s", match.Pattern) + } + } + + if goModMatches != 3 { + t.Errorf("expected 3 go.mod matches, got %d", goModMatches) + } + + if logMatches != 1 { + t.Errorf("expected 1 error log match, got %d", logMatches) + } +} + func TestTiCDCExclusionScenario(t *testing.T) { // Create a test configuration similar to pingcap/ticdc config := &Config{