Refactor required approvals workflow to use custom script.#5134
Refactor required approvals workflow to use custom script.#5134RishabhAgarwal-2001 wants to merge 1 commit intomainfrom
Conversation
…ODEOWNERS pull request approval.
RishabhAgarwal-2001
requested review from
AmrNJ,
kapilrajput-g and
nikki-blr
Summary of ChangesHello @RishabhAgarwal-2001, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the existing required approvals workflow by introducing a new custom Go script. This script automates the process of verifying that all necessary approvals, as defined in the CODEOWNERS file, have been obtained for a pull request. It integrates with the GitHub CLI to retrieve PR data and the GitHub API to resolve team memberships, providing a robust and flexible solution for approval enforcement. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Go script to automate checking for required approvals based on the CODEOWNERS file. The implementation is well-structured and includes a good set of unit tests.
My review has identified a critical issue in error handling that could cause the script to silently fail to enforce all ownership rules. I also found a high-severity issue regarding an incomplete implementation of the CODEOWNERS pattern matching syntax, which could lead to incorrect owner resolution. Additionally, I've provided a couple of medium-severity suggestions to improve code maintainability and readability. Addressing these points will make the script more robust and reliable.
| if err != nil { | ||
| log.Printf("CODEOWNERS not found or error reading: %v", err) | ||
| } |
There was a problem hiding this comment.
If parsing the CODEOWNERS file fails, the error is logged, but the program continues with an empty set of rules. This means the script will only check for the hardcoded default approvers and may incorrectly report success for a PR that is missing required approvals from other code owners.
Failure to read the CODEOWNERS file should be a fatal error to prevent the check from passing with incomplete information.
if err != nil {
log.Fatalf("CODEOWNERS not found or error reading, cannot reliably check approvals: %v", err)
}| func matchOwners(filePath string, rules []Rule) []string { | ||
| var matchedOwners []string | ||
| for _, rule := range rules { | ||
| pattern := rule.Pattern | ||
| owners := rule.Owners | ||
| isMatch := false | ||
|
|
||
| if strings.HasPrefix(pattern, "/") && strings.HasSuffix(pattern, "/") { | ||
| prefix := pattern[1:] | ||
| if strings.HasPrefix(filePath, prefix) { | ||
| isMatch = true | ||
| } | ||
| } else if pattern == "*" { | ||
| isMatch = true | ||
| } else if strings.HasPrefix(pattern, "/") && filePath == pattern[1:] { | ||
| isMatch = true | ||
| } | ||
|
|
||
| if isMatch { | ||
| matchedOwners = owners | ||
| } | ||
| } | ||
| return matchedOwners | ||
| } |
There was a problem hiding this comment.
The matchOwners function provides a very limited implementation of CODEOWNERS pattern matching. It currently supports *, exact file paths from the root (e.g., /path/to/file), and directory prefixes (e.g., /path/).
However, it does not support other standard gitignore-style patterns that CODEOWNERS files use, such as:
- File extensions (
*.go) - Directory wildcards (
docs/**) - Matching directories by name anywhere in the path (
foo/)
This limitation could lead to incorrect owner resolution if the CODEOWNERS file uses more complex patterns, potentially bypassing required reviews. Consider using a library that properly implements .gitignore pattern matching, such as github.com/sabhiram/go-gitignore, to ensure full compatibility.
| type prFiles struct { | ||
| Files []struct { | ||
| Path string `json:"path"` | ||
| } `json:"files"` | ||
| } | ||
|
|
||
| // review represents a single review on a PR. | ||
| type review struct { | ||
| Author struct { | ||
| Login string `json:"login"` | ||
| } `json:"author"` | ||
| State string `json:"state"` | ||
| SubmittedAt string `json:"submittedAt"` | ||
| } |
There was a problem hiding this comment.
The prFiles and review structs use anonymous structs for nested data (Files and Author). Using named structs (e.g., fileInfo, authorInfo) would make the code cleaner, more readable, and simplify test data creation in main_test.go.
type prFiles struct {
Files []fileInfo `json:"files"`
}
type fileInfo struct {
Path string `json:"path"`
}
// review represents a single review on a PR.
type review struct {
Author authorInfo `json:"author"`
State string `json:"state"`
SubmittedAt string `json:"submittedAt"`
}
type authorInfo struct {
Login string `json:"login"`
}| if owner != "@openconfig/featureprofiles-approvers" { | ||
| requiredTeams[owner] = true | ||
| } |
There was a problem hiding this comment.
The logic to populate requiredTeams can be simplified. The if condition to skip adding @openconfig/featureprofiles-approvers is redundant because this team is already added to the map, and maps inherently handle duplicate keys. Removing the condition would make the code's intent clearer and more maintainable.
requiredTeams[owner] = true3 participants
No description provided.