feat: Add re-request to same team when only 1 team is required, but we need multiple #77
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
/gemini review |
![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 useful feature to re-request reviews from a satisfied team when min_reviews is not met. This is particularly helpful for workflows requiring multiple reviews from a single team. The implementation is generally solid, with new logic in app.go and corresponding changes in the codeowners package and tests. I've provided a couple of suggestions to enhance efficiency and reduce code duplication for better long-term maintainability.
internal/app/app.go
Outdated
| // Filter out anyone already requested | ||
| ownersToReRequest := f.Filtered(allRequiredOwnerNames, func(owner string) bool { | ||
| return !slices.Contains(currentlyRequestedOwners, owner) | ||
| }) |
There was a problem hiding this comment.
The current implementation for filtering ownersToReRequest uses slices.Contains inside a loop (f.Filtered), which results in a time complexity of O(N*M). For larger lists of owners, this could be inefficient. A more performant and idiomatic Go approach is to use a map (acting as a set) for currentlyRequestedOwners to achieve O(1) lookups during filtering.
// Filter out anyone already requested
currentlyRequestedSet := make(map[string]struct{}, len(currentlyRequestedOwners))
for _, owner := range currentlyRequestedOwners {
currentlyRequestedSet[owner] = struct{}{}
}
ownersToReRequest := f.Filtered(allRequiredOwnerNames, func(owner string) bool {
_, exists := currentlyRequestedSet[owner]
return !exists
})
pkg/codeowners/codeowners.go
Outdated
| func (om *ownersMap) AllRequiredIncludingSatisfied() ReviewerGroups { | ||
| reviewers := make([]*ReviewerGroup, 0) | ||
| for _, fileOwner := range om.fileToOwner { | ||
| reviewers = append(reviewers, fileOwner.requiredReviewers...) | ||
| } | ||
| return f.RemoveDuplicates(reviewers) | ||
| } |
There was a problem hiding this comment.
This new function AllRequiredIncludingSatisfied is very similar to the existing AllRequired function. To improve maintainability and reduce code duplication, you could consider refactoring them to use a common private helper function.
For example, you could introduce a helper:
func (om *ownersMap) allReviewers(getReviewers func(fileOwners) ReviewerGroups) ReviewerGroups {
reviewers := make([]*ReviewerGroup, 0)
for _, fileOwner := range om.fileToOwner {
reviewers = append(reviewers, getReviewers(fileOwner)...)
}
return f.RemoveDuplicates(reviewers)
}And then AllRequired and AllRequiredIncludingSatisfied would become one-liners:
func (om *ownersMap) AllRequired() ReviewerGroups {
return om.allReviewers(func(fo fileOwners) ReviewerGroups { return fo.RequiredReviewers() })
}
func (om *ownersMap) AllRequiredIncludingSatisfied() ReviewerGroups {
return om.allReviewers(func(fo fileOwners) ReviewerGroups { return fo.requiredReviewers })
}
internal/app/app.go
Outdated
| if validApprovalCount < *a.Conf.MinReviews && len(unapprovedOwners) == 0 { | ||
| // All required teams have approved, but we need more reviews | ||
| // Re-request review from the satisfied team(s) | ||
| allRequiredOwnerNames := a.codeowners.AllRequiredIncludingSatisfied().Flatten() |
There was a problem hiding this comment.
This is totally unnecessary - allRequiredOwnersNames already exists and is never mutated
There was a problem hiding this comment.
ahh, this is was very confusing to me since method used to get allRequiredOwnersNames filters out approved, but didn't realize that it was calculated before the approvals were processed
internal/app/app.go
Outdated
|
|
||
| // Check if we need to re-request from a satisfied team when min_reviews is not met | ||
| // This handles the case where only 1 team is required but multiple reviews are needed | ||
| if a.Conf.MinReviews != nil && *a.Conf.MinReviews > 0 { |
There was a problem hiding this comment.
Can we move this into the existing MinReviews handling block
|
Codeowners approval required for this PR:
|
…lc/codeowners-plus into feature/re-request-only-team
Related PR(s)
Related Issue(s)
#40
Summary / Background
This PR adds checking for when all of teh teams have approved a PR, but there are not enough approvals to satisfy minReviews. In that case, mm-codeowners will re-request from all required teams.
Most commonly, we'll have 2 reviews required and only 1 owning team. That team will then be re-requested for a 2nd review.
If you require 3 reviews, and 2 teams are required, and they've both approved, then both teams will be re-requested.
Important to note that none of this re-requesting logic triggers if there are any outstanding teams still to review, since it checks that there's no outstanding review requests.