Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix review dismissals bug #485

Merged
merged 6 commits into from
Oct 26, 2022
Merged

Fix review dismissals bug #485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05ec17b
only dismiss edited or invalidated reviews
devinburnette Oct 13, 2022
6168933
add patterns to details page
devinburnette Oct 13, 2022
384abc1
implement feedback
devinburnette Oct 15, 2022
c7acebb
Restore custom Requires type in disapproval
bluekeyes Oct 26, 2022
33bf77a
Add nil check
bluekeyes Oct 26, 2022
4661ff0
Fix typo in field name
bluekeyes Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions policy/approval/approve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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,
}
Expand Down Expand Up @@ -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")
}
Expand Down
56 changes: 28 additions & 28 deletions policy/approval/approve_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand Down Expand Up @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand Down Expand Up @@ -620,7 +620,7 @@ func TestTrigger(t *testing.T) {
},
},
},
Requires: Requires{
Requires: common.Requires{
Count: 1,
},
}
Expand All @@ -638,7 +638,7 @@ func TestTrigger(t *testing.T) {
},
},
},
Requires: Requires{
Requires: common.Requires{
Count: 1,
},
}
Expand All @@ -655,7 +655,7 @@ func TestTrigger(t *testing.T) {
GithubReview: &defaultGithubReview,
},
},
Requires: Requires{
Requires: common.Requires{
Count: 1,
},
}
Expand All @@ -673,7 +673,7 @@ func TestTrigger(t *testing.T) {
},
},
},
Requires: Requires{
Requires: common.Requires{
Count: 1,
},
}
Expand All @@ -692,7 +692,7 @@ func TestTrigger(t *testing.T) {
},
IgnoreEditedComments: false,
},
Requires: Requires{
Requires: common.Requires{
Count: 1,
},
}
Expand Down
2 changes: 1 addition & 1 deletion policy/approval/evaluate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func TestRules(t *testing.T) {
GithubReview: &defaultGithubReview,
},
},
Requires: Requires{
Requires: common.Requires{
Count: 5,
Actors: common.Actors{
Users: []string{"user4"},
Expand Down
8 changes: 7 additions & 1 deletion policy/common/result.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,20 @@ type ReviewRequestRule struct {
Mode RequestMode
}

type Requires struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of duplicating Requires in this package, I think it might be better to move approval.Requires to this package and then fix the references in the approval packages. That should reduce confusion between the two types in the future since they're otherwise the same.

Count int `yaml:"count"`
Actors Actors `yaml:",inline"`
}

type Result struct {
Name string
Description string
StatusDescription string
Status EvaluationStatus
Error error
PredicateResults []*PredicateResult
Requires Actors
Requires Requires
Methods *Methods

ReviewRequestRule *ReviewRequestRule
AllowedCandidates []*Candidate
Expand Down
4 changes: 3 additions & 1 deletion policy/disapproval/disapprove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
Expand Down Expand Up @@ -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

Expand Down
11 changes: 7 additions & 4 deletions server/handler/eval_context_dismissal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
Loading