dont evaluate unless we need to #472
Conversation
server/handler/issue_comment.go
Outdated
| @@ -100,6 +100,10 @@ func (h *IssueComment) Handle(ctx context.Context, eventType, deliveryID string, | |||
| return nil | |||
| } | |||
|
|
|||
| if !h.affectsApproval(event.GetComment().GetBody(), fc.Config.ApprovalRules) { | |||
There was a problem hiding this comment.
I think this misses an edge case with edited comments. The previous detectAndLogTampering call exits early if the edit was by the same user that authored the original comment. Since affectsApproval is only looking at the current body, if a user edits their own comment to remove an approval, I think this will cause us to skip evaluating the PR (only the old body affects approval, and we don't check that.)
To catch that case, I think we need to look at the previous content for edited events as well, possibly assuming that a comment affects approval if the old content is missing (e.g. in #379, where a GHE bug removed this field.)
As a general rule, I want to make sure this type of optimization will over-evaluate if there are errors or uncertainty, rather than under-evaluate.
|
|
||
| func (h *PullRequestReview) affectsApproval(reviewState string, rules []*approval.Rule) bool { | ||
| for _, rule := range rules { | ||
| if reviewState == string(rule.Options.GetMethods().GithubReviewState) { |
There was a problem hiding this comment.
nit: it might be better to cast the parameter before calling this function, rather than casting the rule value
| if !h.affectsApproval(event.GetReview().GetState(), fc.Config.ApprovalRules) { | ||
| return nil | ||
| } | ||
|
|
||
| return h.Evaluate(ctx, installationID, common.TriggerReview, pull.Locator{ |
There was a problem hiding this comment.
Sorry about all the boilerplate you have to copy to get access to the config prior to calling Evaluate. But now that we have a FetchedConfig, this should probably call ValidateFetchedConfig and EvaluateFetchedConfig instead, since Evaluate repeats the client creation and config fetching.
server/handler/issue_comment.go
Outdated
| for _, rule := range rules { | ||
| if rule.Options.GetMethods().CommentMatches(actualComment) { | ||
| if rule.Options.GetMethods().CommentMatches(originalBody) { |
There was a problem hiding this comment.
how about this? this will look at all events but only run them through evaluation if the current or previous matches a rule pattern.
There was a problem hiding this comment.
I think this is still missing a case: when the action is edited we need to test both the original body and the new body to catch events where an approval was removed or added.
There was a problem hiding this comment.
oh right, i see what you mean now.
| case "deleted": | ||
| originalBody = event.GetComment().GetBody() | ||
|
|
||
| default: |
There was a problem hiding this comment.
tried to dry this up a bit.
server/handler/issue_comment.go
Outdated
| for _, rule := range rules { | ||
| if rule.Options.GetMethods().CommentMatches(actualComment) { | ||
| if rule.Options.GetMethods().CommentMatches(originalBody) { |
There was a problem hiding this comment.
I think this is still missing a case: when the action is edited we need to test both the original body and the new body to catch events where an approval was removed or added.
| client, err := h.NewInstallationClient(installationID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| v4client, err := h.NewInstallationV4Client(installationID) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
In #476, I did some refactoring to reduce this boiler plate
Fix conflicts from EvalContext refactor
Also add some log messages if we're skipping due to lack of impact
|
lgtm, thanks |
closes #470
As mentioned in #470 there are certain situations where we could run into API limitations on the number of status checks posted to a SHA / context pair. This should cut down on some unnecessary API calls being made currently during evaluation.