diff --git a/policy/approval/approve.go b/policy/approval/approve.go index c39fc7a4..76575097 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -33,7 +33,7 @@ type Rule struct { Description string `yaml:"description"` Predicates predicate.Predicates `yaml:"if"` Options Options `yaml:"options"` - Requires Requires `yaml:"requires"` + Requires common.Requires `yaml:"requires"` } type Options struct { @@ -76,12 +76,6 @@ func (opts *Options) GetMethods() *common.Methods { return methods } -type Requires struct { - Count int `yaml:"count"` - - common.Actors `yaml:",inline"` -} - func (r *Rule) Trigger() common.Trigger { t := common.TriggerCommit @@ -111,7 +105,8 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res res.Name = r.Name res.Description = r.Description res.Status = common.StatusSkipped - res.Requires = r.Requires.Actors + res.Requires = r.Requires + res.Methods = r.Options.GetMethods() var predicateResults []*common.PredicateResult @@ -173,10 +168,10 @@ func (r *Rule) getReviewRequestRule() *common.ReviewRequestRule { } return &common.ReviewRequestRule{ - Users: r.Requires.Users, - Teams: r.Requires.Teams, - Organizations: r.Requires.Organizations, - Permissions: r.Requires.GetPermissions(), + Users: r.Requires.Actors.Users, + Teams: r.Requires.Actors.Teams, + Organizations: r.Requires.Actors.Organizations, + Permissions: r.Requires.Actors.GetPermissions(), RequiredCount: r.Requires.Count, Mode: mode, } @@ -227,7 +222,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates [] continue } - isApprover, err := r.Requires.IsActor(ctx, prctx, c.User) + isApprover, err := r.Requires.Actors.IsActor(ctx, prctx, c.User) if err != nil { return false, "", errors.Wrap(err, "failed to check candidate status") } diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index 31c18227..91e8fb56 100644 --- a/policy/approval/approve_test.go +++ b/policy/approval/approve_test.go @@ -173,7 +173,7 @@ func TestIsApproved(t *testing.T) { t.Run("singleApprovalRequired", func(t *testing.T) { prctx := basePullContext() r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, }, } @@ -186,7 +186,7 @@ func TestIsApproved(t *testing.T) { Options: Options{ AllowAuthor: false, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"everyone"}, @@ -202,7 +202,7 @@ func TestIsApproved(t *testing.T) { Options: Options{ AllowAuthor: true, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"everyone"}, @@ -218,7 +218,7 @@ func TestIsApproved(t *testing.T) { Options: Options{ AllowContributor: false, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"everyone"}, @@ -235,7 +235,7 @@ func TestIsApproved(t *testing.T) { AllowContributor: true, AllowAuthor: false, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"everyone"}, @@ -251,7 +251,7 @@ func TestIsApproved(t *testing.T) { Options: Options{ AllowNonAuthorContributor: true, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"everyone"}, @@ -268,7 +268,7 @@ func TestIsApproved(t *testing.T) { AllowNonAuthorContributor: true, AllowAuthor: true, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"everyone"}, @@ -285,7 +285,7 @@ func TestIsApproved(t *testing.T) { AllowNonAuthorContributor: true, AllowContributor: true, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"everyone"}, @@ -298,7 +298,7 @@ func TestIsApproved(t *testing.T) { t.Run("specificUserApproves", func(t *testing.T) { prctx := basePullContext() r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"comment-approver"}, @@ -308,7 +308,7 @@ func TestIsApproved(t *testing.T) { assertApproved(t, prctx, r, "Approved by comment-approver") r = &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"does-not-exist"}, @@ -321,7 +321,7 @@ func TestIsApproved(t *testing.T) { t.Run("specificOrgApproves", func(t *testing.T) { prctx := basePullContext() r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"cool-org"}, @@ -331,7 +331,7 @@ func TestIsApproved(t *testing.T) { assertApproved(t, prctx, r, "Approved by comment-approver") r = &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Organizations: []string{"does-not-exist", "other-org"}, @@ -344,7 +344,7 @@ func TestIsApproved(t *testing.T) { t.Run("specificOrgsOrUserApproves", func(t *testing.T) { prctx := basePullContext() r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 2, Actors: common.Actors{ Users: []string{"review-approver"}, @@ -367,7 +367,7 @@ func TestIsApproved(t *testing.T) { } r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"comment-approver"}, @@ -392,7 +392,7 @@ func TestIsApproved(t *testing.T) { } r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"review-approver"}, @@ -419,7 +419,7 @@ func TestIsApproved(t *testing.T) { }) r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"comment-approver"}, @@ -454,7 +454,7 @@ func TestIsApproved(t *testing.T) { }) r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"merge-committer"}, @@ -476,7 +476,7 @@ func TestIsApproved(t *testing.T) { }) r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"comment-approver"}, @@ -495,7 +495,7 @@ func TestIsApproved(t *testing.T) { prctx := basePullContext() r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"contributor-author"}, @@ -522,7 +522,7 @@ func TestIsApproved(t *testing.T) { } r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"comment-approver"}, @@ -542,7 +542,7 @@ func TestIsApproved(t *testing.T) { prctx := basePullContext() r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"review-comment-editor"}, @@ -561,7 +561,7 @@ func TestIsApproved(t *testing.T) { prctx := basePullContext() r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"comment-editor"}, @@ -580,7 +580,7 @@ func TestIsApproved(t *testing.T) { prctx := basePullContext() r := &Rule{ - Requires: Requires{ + Requires: common.Requires{ Count: 1, Actors: common.Actors{ Users: []string{"body-editor"}, @@ -620,7 +620,7 @@ func TestTrigger(t *testing.T) { }, }, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, }, } @@ -638,7 +638,7 @@ func TestTrigger(t *testing.T) { }, }, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, }, } @@ -655,7 +655,7 @@ func TestTrigger(t *testing.T) { GithubReview: &defaultGithubReview, }, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, }, } @@ -673,7 +673,7 @@ func TestTrigger(t *testing.T) { }, }, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, }, } @@ -692,7 +692,7 @@ func TestTrigger(t *testing.T) { }, IgnoreEditedComments: false, }, - Requires: Requires{ + Requires: common.Requires{ Count: 1, }, } diff --git a/policy/approval/evaluate_test.go b/policy/approval/evaluate_test.go index 56647b67..b03982a0 100644 --- a/policy/approval/evaluate_test.go +++ b/policy/approval/evaluate_test.go @@ -107,7 +107,7 @@ func TestRules(t *testing.T) { GithubReview: &defaultGithubReview, }, }, - Requires: Requires{ + Requires: common.Requires{ Count: 5, Actors: common.Actors{ Users: []string{"user4"}, diff --git a/policy/common/result.go b/policy/common/result.go index 99ed3d2d..fef0d90a 100644 --- a/policy/common/result.go +++ b/policy/common/result.go @@ -59,6 +59,11 @@ type ReviewRequestRule struct { Mode RequestMode } +type Requires struct { + Count int `yaml:"count"` + Actors Actors `yaml:",inline"` +} + type Result struct { Name string Description string @@ -66,7 +71,8 @@ type Result struct { Status EvaluationStatus Error error PredicateResults []*PredicateResult - Requires Actors + Requires Requires + Methods *Methods ReviewRequestRule *ReviewRequestRule AllowedCandidates []*Candidate diff --git a/policy/disapproval/disapprove.go b/policy/disapproval/disapprove.go index 7c46a7cc..c1917230 100644 --- a/policy/disapproval/disapprove.go +++ b/policy/disapproval/disapprove.go @@ -75,6 +75,8 @@ func (opts *Options) GetRevokeMethods() *common.Methods { return m } +// Requires is redefined instead of using common.Requires because disapproval +// does not currently support required counts. type Requires struct { common.Actors `yaml:",inline"` } @@ -106,7 +108,7 @@ func (p *Policy) Evaluate(ctx context.Context, prctx pull.Context) (res common.R res.Name = "disapproval" res.Status = common.StatusSkipped - res.Requires = p.Requires.Actors + res.Requires = common.Requires{Actors: p.Requires.Actors} var predicateResults []*common.PredicateResult diff --git a/server/handler/eval_context_dismissal.go b/server/handler/eval_context_dismissal.go index 8f92c35c..5375510e 100644 --- a/server/handler/eval_context_dismissal.go +++ b/server/handler/eval_context_dismissal.go @@ -56,6 +56,10 @@ func (ec *EvalContext) dismissStaleReviewsForResult(ctx context.Context, result } reason := reasonForDismissedReview(r) + if reason == "" { + continue + } + message := fmt.Sprintf("Dismissed because the approval %s", reason) logger.Info().Msgf("Dismissing stale review %s because it %s", r.ID, reason) if err := dismissPullRequestReview(ctx, ec.V4Client, r.ID, message); err != nil { @@ -88,10 +92,9 @@ func reviewIsAllowed(review *pull.Review, allowedCandidates []*common.Candidate) return false } -// We already know that these are discarded review candidates for one of three reasons +// We already know that these are discarded review candidates for 1 of 2 reasons // so first we check for edited and then we check to see if its a review thats at least -// 5 seconds old and we know that it was invalidated by a new commit, and then finally -// if it was missing a github review comment pattern that was required. +// 5 seconds old and we know that it was invalidated by a new commit. // // This is brittle and may need refactoring in future versions because it assumes the bot // will take less than 5 seconds to respond, but thought that having a dismissal reason @@ -105,7 +108,7 @@ func reasonForDismissedReview(review *pull.Review) string { return "was invalidated by another commit" } - return "didn't include a valid comment pattern" + return "" } func dismissPullRequestReview(ctx context.Context, v4client *githubv4.Client, reviewID string, message string) error { diff --git a/server/handler/frontend.go b/server/handler/frontend.go index 20fa7f82..a1772b27 100644 --- a/server/handler/frontend.go +++ b/server/handler/frontend.go @@ -60,14 +60,17 @@ func LoadTemplates(c *FilesConfig, basePath string, githubURL string) (templatet return r }, - "hasRequires": func(requires common.Actors) bool { - return len(requires.Users) > 0 || len(requires.Teams) > 0 || len(requires.Organizations) > 0 + "hasActors": func(requires *common.Requires) bool { + return len(requires.Actors.Users) > 0 || len(requires.Actors.Teams) > 0 || len(requires.Actors.Organizations) > 0 }, - "getRequires": func(results *common.Result) map[string][]Membership { - return getRequires(results, strings.TrimSuffix(githubURL, "/")) + "getMethods": func(results *common.Result) map[string][]string { + return getMethods(results) }, - "hasRequiresPermissions": func(requires common.Actors) bool { - return len(requires.GetPermissions()) > 0 + "getActors": func(results *common.Result) map[string][]Membership { + return getActors(results, strings.TrimSuffix(githubURL, "/")) + }, + "hasActorsPermissions": func(requires *common.Requires) bool { + return len(requires.Actors.GetPermissions()) > 0 }, "getPermissions": func(results *common.Result) []string { return getPermissions(results) @@ -91,24 +94,47 @@ func Static(prefix string, c *FilesConfig) http.Handler { return http.StripPrefix(prefix, http.FileServer(http.Dir(dir))) } -func getRequires(result *common.Result, githubURL string) map[string][]Membership { +func getMethods(result *common.Result) map[string][]string { + patternInfo := make(map[string][]string) + for _, comment := range result.Methods.Comments { + patternInfo["Comments"] = append(patternInfo["Comments"], comment) + } + for _, commentPattern := range result.Methods.CommentPatterns { + patternInfo["Comment Patterns"] = append(patternInfo["Comment Patterns"], commentPattern.String()) + } + for _, bodyPattern := range result.Methods.BodyPatterns { + patternInfo["Body Patterns"] = append(patternInfo["Body Patterns"], bodyPattern.String()) + } + if result.Methods.GithubReview != nil && *result.Methods.GithubReview { + if len(result.Methods.GithubReviewCommentPatterns) > 0 { + for _, githubReviewCommentPattern := range result.Methods.GithubReviewCommentPatterns { + patternInfo["Github Review Comment Patterns + Github Review Approval"] = append(patternInfo["Github Review Comment Patterns + Github Review Approval"], githubReviewCommentPattern.String()) + } + } else { + patternInfo["Github Review State"] = append(patternInfo["Github Review State"], string(result.Methods.GithubReviewState)) + } + } + return patternInfo +} + +func getActors(result *common.Result, githubURL string) map[string][]Membership { membershipInfo := make(map[string][]Membership) - for _, org := range result.Requires.Organizations { + for _, org := range result.Requires.Actors.Organizations { membershipInfo["Organizations"] = append(membershipInfo["Organizations"], Membership{Name: org, Link: githubURL + "/orgs/" + org + "/people"}) } - for _, team := range result.Requires.Teams { + for _, team := range result.Requires.Actors.Teams { teamName := strings.Split(team, "/") membershipInfo["Teams"] = append(membershipInfo["Teams"], Membership{Name: team, Link: githubURL + "/orgs/" + teamName[0] + "/teams/" + teamName[1] + "/members"}) } - for _, user := range result.Requires.Users { + for _, user := range result.Requires.Actors.Users { membershipInfo["Users"] = append(membershipInfo["Users"], Membership{Name: user, Link: githubURL + "/" + user}) } return membershipInfo } func getPermissions(result *common.Result) []string { - perms := result.Requires.GetPermissions() + perms := result.Requires.Actors.GetPermissions() permStrings := make([]string, 0, len(perms)) for _, perm := range perms { permStrings = append(permStrings, perm.String()) diff --git a/server/templates/details.html.tmpl b/server/templates/details.html.tmpl index 2791e5b4..08efed9e 100644 --- a/server/templates/details.html.tmpl +++ b/server/templates/details.html.tmpl @@ -56,7 +56,7 @@
  • {{template "result-details" .}} - {{if (or (.PredicateResults) (hasRequires .Requires) (hasRequiresPermissions .Requires))}} + {{if (or (.PredicateResults) (hasActors .Requires) (hasActorsPermissions .Requires))}}
    Details
    @@ -65,16 +65,22 @@ {{template "result-predicates-details" .}}
    {{end}} - {{if (hasRequires .Requires)}} +
    + {{template "result-requires-count" .}} +
    + {{if (hasActors .Requires)}}
    - {{template "result-requires-details" .}} + {{template "result-actors-details" .}}
    {{end}} - {{if (hasRequiresPermissions .Requires)}} + {{if (hasActorsPermissions .Requires)}}
    - {{template "result-requires-permissions-details" .}} + {{template "result-actors-permissions-details" .}}
    {{end}} +
    + {{template "result-methods-details" .}} +
    {{end}} @@ -142,10 +148,29 @@ {{end}} -{{define "result-requires-details"}} +{{define "result-requires-count"}} + This rule requires at least this many reviews +
    +
    {{.Requires.Count}}
    +
    +{{end}} + +{{define "result-methods-details"}} + This rule requires review using any of these methods +
    + {{range $k, $v := getMethods .}} +
    {{$k}}
    +
    +
      {{range $v}}
    • {{.}}
      • {{end}}
    +
    + {{end}} +
    +{{end}} + +{{define "result-actors-details"}} This rule requires review from
    - {{range $k, $v := getRequires .}} + {{range $k, $v := getActors .}}
    {{$k}}
    @@ -154,8 +179,8 @@
    {{end}} -{{define "result-requires-permissions-details"}} - This rule requires review from users with the following permissions +{{define "result-actors-permissions-details"}} + This rule requires review from users with these permissions