feat: add comment matching to github review approval with new option #361
Conversation
policy/common/methods.go
Outdated
| Comments []string `yaml:"comments,omitempty"` | ||
| CommentPatterns []Regexp `yaml:"comment_patterns,omitempty"` | ||
| GithubReview bool `yaml:"github_review,omitempty"` | ||
| GithubReviewComments []string `yaml:"github_review_comments,omitempty"` |
There was a problem hiding this comment.
Let's remove this option and only support GitHubReviewCommentPatterns.
If I were to implement Policy Bot again (or make a v2 configuration format), I think I'd only support CommentPatterns, since it's a superset of the plain Comments field. While we haven't formally deprecated Comments, I consider it deprecated.
There was a problem hiding this comment.
makes sense, fine by me.
policy/common/methods.go
Outdated
| for _, comment := range m.GithubReviewComments { | ||
| if strings.Contains(commentBody, comment) { | ||
| return true | ||
| } | ||
| } | ||
| for _, pattern := range m.GithubReviewCommentPatterns { | ||
| if pattern.Matches(commentBody) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe this will have to be in a new method so that review comment patterns are not matched against regular comment patterns and vice versa.
policy/common/methods.go
Outdated
| CreatedAt: r.CreatedAt, | ||
| UpdatedAt: r.UpdatedAt, | ||
| }) | ||
| if len(m.GithubReviewComments) > 0 || len(m.GithubReviewCommentPatterns) > 0 { |
There was a problem hiding this comment.
Because of the way method defaults work, I don't think this will work as expected (or at least as we discussed in the issue.) We only use the defaults if the methods block is completely absent. If the user sets any fields, no defaults are used. That means that with the current code, users will have to set both github_reviews and github_review_comment_patterns to have things work.
We may want to change the condition on L76 to m.GitubReview || len(m.GithubReviewCommandPatterns) > 0 so that setting either field will enable review processing - unless we actually want to require that both fields are set.
There was a problem hiding this comment.
ahh ok, i think i misunderstood how the defaults were applied. Lemme look into fixing this.
devinburnette
force-pushed
the
review-approval-with-comments
branch
3 times, most recently
from
e9a64ea to
7f8ee6b
Compare
devinburnette
force-pushed
the
review-approval-with-comments
branch
from
7f8ee6b to
4f06a5f
Compare
| # review approvals matching the included patterns will be accepted as | ||
| # approval candidates. Defaults to an empty list. | ||
| github_review_comment_patterns: | ||
| - '\b(?i)domain\s*lgtm\b' |
There was a problem hiding this comment.
@bluekeyes ok! I think this is all set. I've also updated the readme to reflect the use case more accurately.
Closes #359
As discussed in the attached issue, this feature would support new method options for
github_review_comments:andgithub_review_comment_patterns:creating parity with issue comment approval for github review approval comment bodies. This would help in use cases where github review approval alone isn't sufficient.I went with nesting the logic in the existing reviews for loop but first checking to confirm the review state is approved. That would make this feature work if
github_reviewis provided in the config and set totrue, or not provided in the config defaulting totrue. As written, this feature doesn't work ifgithub_review: falseexplicitly in the config. That doesn't seem surprising behavior to me, but let me know if there's an approach that makes better sense to accomplish the same.