diff --git a/internal/api/approval/approval.go b/internal/api/approval/approval.go index 46b525f..2e108df 100644 --- a/internal/api/approval/approval.go +++ b/internal/api/approval/approval.go @@ -75,10 +75,67 @@ func (a *Approval) ComputeApprovalStatus(ctx context.Context, pr *PR) (*Result, return nil, err } - rules, err := a.computeRulesForTargetBranch(cfg, pr) + // Grab the list of teams under the current organisation. + teams, err := a.client.GetTeams(ctx, pr.OwnerLogin) + if err != nil { + return nil, err + } + + // Grab the list of all the reviews for the current PR. + reviews, err := a.client.GetPullRequestReviews(ctx, pr.OwnerLogin, pr.RepoName, pr.Number) if err != nil { return nil, err } + + results := make([]*Result, 0) + for _, prCfg := range cfg.PullRequestApprovalRules { + if len(prCfg.TargetBranches) == 0 || indexOf(prCfg.TargetBranches, pr.TargetBranch) >= 0 { + result, err := a.computeApprovalStatus(ctx, pr, teams, reviews, prCfg.Rules) + if err != nil { + return nil, err + } + + results = append(results, result) + } + } + + if len(results) == 0 { + return &Result{ + status: StatusEventStatusSuccess, + description: statusEventDescriptionNoRulesForTargetBranch, + }, nil + } + + firstFailedResult := firstFailedResult(results) + if firstFailedResult != nil { + ignoredReviewers := results[0].IgnoredReviewers() + invalidReviewers := results[0].InvalidReviewers() + for _, result := range results[1:] { + ignoredReviewers = intersection(ignoredReviewers, result.IgnoredReviewers()) + invalidReviewers = intersection(invalidReviewers, result.InvalidReviewers()) + } + + if err := a.client.ReportIgnoredReviews(ctx, pr.OwnerLogin, pr.RepoName, pr.Number, ignoredReviewers); err != nil { + return nil, err + } + if err := a.client.ReportInvalidReviews(ctx, pr.OwnerLogin, pr.RepoName, pr.Number, invalidReviewers); err != nil { + return nil, err + } + } + + return firstFailedResult, nil +} + +func firstFailedResult(results []*Result) *Result { + for _, result := range results { + if result.Status() != StatusEventStatusSuccess { + return result + } + } + return nil +} + +func (a *Approval) computeApprovalStatus(ctx context.Context, pr *PR, teams []*github.Team, reviews []*github.PullRequestReview, rules []configuration.Rule) (*Result, error) { if len(rules) > 0 { a.log.Tracef("A total of %d rules apply to target branch %q", len(rules), pr.TargetBranch) } else { @@ -91,18 +148,6 @@ func (a *Approval) ComputeApprovalStatus(ctx context.Context, pr *PR) (*Result, return status, nil } - // Grab the list of teams under the current organisation. - teams, err := a.client.GetTeams(ctx, pr.OwnerLogin) - if err != nil { - return nil, err - } - - // Grab the list of all the reviews for the current PR. - reviews, err := a.client.GetPullRequestReviews(ctx, pr.OwnerLogin, pr.RepoName, pr.Number) - if err != nil { - return nil, err - } - state := newState() state.setApprovingReviewers(reviews) @@ -168,21 +213,7 @@ func (a *Approval) ComputeApprovalStatus(ctx context.Context, pr *PR) (*Result, state.updateInvalidReviewers(allAllowedMembers) - result := state.result(a.log, teams) // state should not be consumed past this point - - if result.pendingReviewsWaiting() { - if err := a.client.ReportIgnoredReviews( - ctx, pr.OwnerLogin, pr.RepoName, pr.Number, result.IgnoredReviewers()); err != nil { - return nil, err - } - - if err := a.client.ReportInvalidReviews( - ctx, pr.OwnerLogin, pr.RepoName, pr.Number, result.InvalidReviewers()); err != nil { - return nil, err - } - } - - return result, nil + return state.result(a.log, teams), nil // state should not be consumed past this point } func addMembers(allowed map[string]bool, members []*github.User) { diff --git a/internal/api/approval/string_slices.go b/internal/api/approval/string_slices.go index b0b1bb8..dd6e66e 100644 --- a/internal/api/approval/string_slices.go +++ b/internal/api/approval/string_slices.go @@ -39,3 +39,18 @@ func indexOf(s []string, v string) int { } return -1 } + +func intersection(a, b []string) []string { + m := map[string]bool{} + for _, e := range a { + m[e] = true + } + result := make([]string, 0, len(a)) + for _, e := range b { + if _, ok := m[e]; ok { + result = append(result, e) + delete(m, e) // avoid duplicates + } + } + return result +} diff --git a/internal/api/approval/string_slices_test.go b/internal/api/approval/string_slices_test.go index 7a57d48..806a682 100644 --- a/internal/api/approval/string_slices_test.go +++ b/internal/api/approval/string_slices_test.go @@ -153,3 +153,54 @@ func Test_uniqueAppend(t *testing.T) { }) } } + +func Test_intersection(t *testing.T) { + tests := map[string]struct { + a []string + b []string + want []string + }{ + "intersection of nil and empty slice": { + nil, + []string{}, + []string{}, + }, + "intersection of empty slice and nil": { + []string{}, + nil, + []string{}, + }, + "intersection of empty slices": { + []string{}, + []string{}, + []string{}, + }, + "intersection of empty slice and empty slice": { + []string{}, + []string{}, + []string{}, + }, + "intersection of slices with no common elements": { + []string{"foo", "bar", "baz"}, + []string{"qux", "quux", "corge"}, + []string{}, + }, + "intersection of slices with common elements": { + []string{"foo", "bar", "baz"}, + []string{"foo", "bar", "qux"}, + []string{"foo", "bar"}, + }, + "intersection of slices with duplicates": { + []string{"foo", "foo", "bar", "baz"}, + []string{"foo", "bar", "bar", "qux"}, + []string{"foo", "bar"}, + }, + } + for name, tt := range tests { + t.Run(name, + func(t *testing.T) { + require.Equal(t, intersection(tt.a, tt.b), tt.want) + }, + ) + } +}