feat: ignore edited comments with new option #357
Conversation
devinburnette
force-pushed
the
ignore-edited-comments-option
branch
from
22bd9fb
to
7980745
Compare
policy/approval/approve.go
Outdated
| if r.Options.IgnoreEditedComments { | ||
| if c.UpdatedAt != c.CreatedAt { | ||
| log.Debug().Str("user", c.User).Msg("ignoring approval because comment was edited") | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
While I believe this will work here, I think it might be more correct to perform this filtering as part of the filteredCandidates function. That will make sure they are ignored before almost any other processing happens and logically, if we're ignoring these comments, they should never appear as "candidates" for approval.
devinburnette
force-pushed
the
ignore-edited-comments-option
branch
from
7980745
to
ba597fe
Compare
policy/approval/approve.go
Outdated
| if candidate.CreatedAt.After(*last.PushedAt) { | ||
| allowedCandidates = append(allowedCandidates, candidate) | ||
| if r.Options.IgnoreEditedComments { | ||
| if candidate.UpdatedAt != candidate.CreatedAt { | ||
| log.Debug().Msg("discarded candidate because it was an edited comment") | ||
| continue | ||
| } | ||
| } | ||
| if r.Options.InvalidateOnPush { | ||
| if candidate.CreatedAt.Before(*last.PushedAt) { | ||
| log.Debug().Msgf("discarded candidate because it was invalidated by push of %s at %s", | ||
| last.SHA, | ||
| last.PushedAt.Format(time.RFC3339)) | ||
| continue | ||
| } | ||
| } | ||
| allowedCandidates = append(allowedCandidates, candidate) |
There was a problem hiding this comment.
@bluekeyes like this? or were you thinking something different?
There was a problem hiding this comment.
Yeah, this is the right direction, but I think it needs a few more adjustments.
policy/approval/approve.go
Outdated
| if candidate.CreatedAt.After(*last.PushedAt) { | ||
| allowedCandidates = append(allowedCandidates, candidate) | ||
| if r.Options.IgnoreEditedComments { | ||
| if candidate.UpdatedAt != candidate.CreatedAt { | ||
| log.Debug().Msg("discarded candidate because it was an edited comment") | ||
| continue | ||
| } | ||
| } | ||
| if r.Options.InvalidateOnPush { | ||
| if candidate.CreatedAt.Before(*last.PushedAt) { | ||
| log.Debug().Msgf("discarded candidate because it was invalidated by push of %s at %s", | ||
| last.SHA, | ||
| last.PushedAt.Format(time.RFC3339)) | ||
| continue | ||
| } | ||
| } | ||
| allowedCandidates = append(allowedCandidates, candidate) |
There was a problem hiding this comment.
Yeah, this is the right direction, but I think it needs a few more adjustments.
policy/approval/approve.go
Outdated
| @@ -256,7 +257,7 @@ func (r *Rule) filteredCandidates(ctx context.Context, prctx pull.Context) ([]*c | |||
| } | |||
| sort.Stable(common.CandidatesByCreationTime(candidates)) | |||
|
|
|||
| if !r.Options.InvalidateOnPush { | |||
| if !r.Options.IgnoreEditedComments && !r.Options.InvalidateOnPush { | |||
There was a problem hiding this comment.
I think we need to treat these as two separate conditions: the code on L264-275 is only relevant for InvalidateOnPush, so ideally we only run that when the option is enabled. There's also a situation (L269) where we could return early without applying IgnoreEditedComments filtering.
It might make sense to extract each filter into a new helper function so that we can directly update candidates, something like:
candidates, err := r.Options.GetMethods().Candidates(ctx, prctx)
// ...
if r.Options.IgnoreEditedComments {
candidates, err = r.filterEditedCandidates(ctx, prctx, candidates)
if err != nil {
return nil, err
}
}
if r.Options.InvalidateOnPush {
candidates, err = r.filterInvalidCandidates(ctx, prctx, candidates)
if err != nil {
return nil, err
}
}
return candidatesThere was a problem hiding this comment.
i see, i missed L269, will give this approach a shot.
There was a problem hiding this comment.
i guess i was just trying to avoid looping over the candidates multiple times
devinburnette
force-pushed
the
ignore-edited-comments-option
branch
from
ba597fe
to
fd663d5
Compare
| func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, error) { | ||
| log := zerolog.Ctx(ctx) | ||
|
|
||
| if !r.Options.IgnoreEditedComments { | ||
| return candidates, nil | ||
| } | ||
|
|
||
| var allowedCandidates []*common.Candidate | ||
| for _, candidate := range candidates { | ||
| if r.Options.IgnoreEditedComments { | ||
| if candidate.UpdatedAt == candidate.CreatedAt { | ||
| allowedCandidates = append(allowedCandidates, candidate) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| log.Debug().Msgf("discarded %d candidates with edited comments", | ||
| len(candidates)-len(allowedCandidates)) | ||
|
|
||
| return allowedCandidates, nil | ||
| } | ||
|
|
||
| func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, error) { | ||
| log := zerolog.Ctx(ctx) | ||
|
|
There was a problem hiding this comment.
@bluekeyes updated with the suggested approach here.
There was a problem hiding this comment.
Thanks for the updates - this looks good to me! I think iterating over candidates multiple times is fine given the expected low number and the relative cost compared to all the API requests that have to be made.
I'll try to test this out in our environment and make a release by the end of the week. If you'd like to test in your environment before that and you're using Docker, you can pull the snapshot tag from Docker Hub once this merges and the build completes.
Closes #356
As discussed in the attached issue, this feature would allow a new boolean option
ignore_edited_comments:that when set totruewould ignore both issue comments and review comments that have been edited when evaluating the approval rules. This helps guard against potential situations where a user could edit a comment to gain approval outside of the intended approval rules construct.I went with adding the
updated_atfield we get back from the graphql response because that makes it easy to determine quickly if a comment has been edited irrespective of whom the comment was edited by (which was also an intentional choice).I went with putting it in the approval part of the code because it seemed like that was where it best fit naturally given we already have access to the rule options and the candidates and this is the place we seem to already be doing other types of candidate filtering. This would have been harder to implement over in the server handlers as we don't have access to the options yet. Let me know what you think of this approach, open to other ideas, but this seemed the easiest to implement.