feat: implements codeowners, teams, PR reviews and Comments #8
Conversation
…y parsing Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
There was a problem hiding this comment.
Pull request overview
This PR implements support for fetching and exposing CODEOWNERS files, organization teams with members, pull request reviews, and review comments/threads for policy evaluation. The implementation adds GraphQL API support alongside the existing REST API client to retrieve review thread data.
Key Changes:
- Adds GraphQL client integration for fetching PR review threads and comments
- Implements organization team and member fetching with permission error handling
- Adds CODEOWNERS file retrieval with fallback path checking
- Enriches pull request data with nested reviews and discussion threads
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| types.go | Defines new data structures for enriched pull requests with reviews/threads, review thread comments, and organization teams |
| pull_requests.go | Implements fetching of PR reviews via REST API and review threads via GraphQL API, with nesting logic to associate threads with reviews |
| org_teams.go | Implements organization team and member fetching with graceful permission error handling |
| codeowners.go | Implements CODEOWNERS file retrieval with multiple path fallback and permission error handling |
| main.go | Integrates new data fetching into the main evaluation flow, adds GraphQL client initialization, and includes a bug fix for pagination handling |
| go.mod | Adds dependencies for GraphQL client (githubv4) and OAuth2 |
| go.sum | Updates checksums for new and updated dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pull_requests.go
Outdated
| func (l *GithubReposPlugin) fetchThreadsForReview(ctx context.Context, reviewNodeID string) ([]*PullRequestReviewThread, error) { | ||
| const pageSize = 50 | ||
| var threads []*PullRequestReviewThread | ||
| var cursor *githubv4.String | ||
|
|
||
| for { | ||
| var query struct { | ||
| Node struct { | ||
| PullRequestReview struct { | ||
| PullRequest struct { | ||
| ReviewThreads struct { | ||
| Nodes []reviewThreadNode | ||
| PageInfo struct { | ||
| HasNextPage githubv4.Boolean | ||
| EndCursor githubv4.String | ||
| } | ||
| } `graphql:"reviewThreads(first: $threadsFirst, after: $threadsCursor)"` | ||
| } `graphql:"pullRequest"` | ||
| } `graphql:"... on PullRequestReview"` | ||
| } `graphql:"node(id: $reviewID)"` | ||
| } | ||
|
|
||
| variables := map[string]interface{}{ | ||
| "reviewID": githubv4.ID(reviewNodeID), | ||
| "threadsFirst": githubv4.Int(pageSize), | ||
| "threadsCursor": cursor, | ||
| } | ||
|
|
||
| if err := l.graphqlClient.Query(ctx, &query, variables); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| threadConnection := query.Node.PullRequestReview.PullRequest.ReviewThreads | ||
| for _, node := range threadConnection.Nodes { | ||
| if !threadIncludesReview(node, reviewNodeID) { | ||
| continue | ||
| } | ||
| threads = append(threads, convertGraphQLThread(node)) | ||
| } |
There was a problem hiding this comment.
The GraphQL query fetches all review threads for the pull request and then filters them client-side for each review. This means if a PR has N reviews, the same threads are fetched N times from GitHub's API. Consider refactoring to fetch all threads for a PR once and then distribute them to their respective reviews, or investigate if there's a GraphQL query to fetch threads for a specific review directly.
pull_requests.go
Outdated
| IsResolved githubv4.Boolean | ||
| Comments struct { | ||
| Nodes []reviewThreadCommentNode `graphql:"nodes"` | ||
| } `graphql:"comments(first: 100)"` |
There was a problem hiding this comment.
The GraphQL query for fetching thread comments has a hardcoded limit of 100 comments per thread without pagination. This could lead to incomplete data if a review thread has more than 100 comments. Consider implementing pagination for comments similar to how it's done for review threads at line 156, or document this limitation if it's an acceptable constraint.
| members, err := l.listTeamMembers(ctx, team.GetSlug()) | ||
| if err != nil { | ||
| l.Logger.Error("Could not get Team members for team", "team", team.GetSlug(), "error", err) | ||
| continue | ||
| } | ||
|
|
||
| teams = append(teams, &OrgTeam{ | ||
| ID: team.GetID(), | ||
| Name: team.GetName(), | ||
| Slug: team.GetSlug(), | ||
| Members: members, | ||
| }) |
There was a problem hiding this comment.
When fetching team members fails, the error is logged but the team is still added with empty Members (nil). This could be misleading for policy evaluation as it's unclear whether the team actually has no members or if member fetching failed. Consider either: 1) skipping the team entirely when member fetch fails, 2) setting Members to an empty slice instead of nil to distinguish from permission errors, or 3) adding a comment documenting this behavior.
|
|
||
| // FetchCodeOwners attempts to retrieve the CODEOWNERS file for the repository. | ||
| // If the file cannot be found or is inaccessible due to permissions, the function | ||
| // returns nil and logs the reason so policy evaluation can proceed with an empty value. |
There was a problem hiding this comment.
The function comment at line 17 states that it "logs the reason" when the file cannot be found or is inaccessible, but the function doesn't actually log anything when returning nil (lines 29, 38). Consider either updating the comment to reflect the actual behavior, or add logging when permission errors occur or when no CODEOWNERS file is found.
| // returns nil and logs the reason so policy evaluation can proceed with an empty value. | |
| // returns nil so policy evaluation can proceed with an empty value. |
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
No description provided.