Add a review to a PR instead of a comment (issue #14)#36
Add a review to a PR instead of a comment (issue #14)#36su7edja wants to merge 5 commits intogoogle:mainfrom
Conversation
mbrukman
changed the title
mbrukman
left a comment
mbrukman
left a comment
There was a problem hiding this comment.
Thanks for your contribution! Can you please add tests to your PR?
If it's hard to do that now (because some methods in this file are large), please note that I'm refactoring this file and adding tests as I go, e.g., see #37 for my latest refactoring; more coming soon. Let me know if something is complex or hard to test, I'm working on improving this and happy to do that to enable you to write tests easier.
Thanks again!
ghutil/ghutil.go
Outdated
|
|
||
| if labelsUpdatedForNonCompliance { | ||
| addComment(pullRequestNonComplianceReason) | ||
| addReview(pullRequestNonComplianceReason, ReviewRequestChanges) |
There was a problem hiding this comment.
Can we have the tool resolve its own review if the user updates their PR such that next time, it's a compliant PR?
|
@mbrukman |
|
@mbrukman finally got the tests working. Let me know what you think. |
mbrukman
left a comment
mbrukman
left a comment
There was a problem hiding this comment.
Thanks for updating the code and adding tests! A few comments / questions below.
| logging.Infof(" No action needed: [%s] label already added", LabelClaYes) | ||
| } | ||
|
|
||
| addReview(pullRequestNonComplianceReason, ReviewApprove) |
There was a problem hiding this comment.
If "ReviewApprove" is going to add an "approval" to the PR in the sense of getting a check mark on the PR, that's a bit of an issue, because the bot having commit permissions on the repo (which appears to be required to add/remove labels) means that just passing the CLA check gets a user a valid green check, and thus making it look like it's ready for merge.
Is there another state we can have the bot take which just resolves their comments on the PR, without approving the PR for the contents, since it's not actually reviewing the meaning of the PR, just CLA compliance?
| }, | ||
| }, | ||
| } | ||
| var prNum = 42 |
There was a problem hiding this comment.
You're using prNum here, but there's already a global const pullNumber which has the same value, and you're using both of them below, so I think you can just drop this line. Similarly in the other test method.
| mockGhc.Issues.EXPECT().AddLabelsToIssue(ctx, orgName, repoName, prNum, []string{"cla: yes"}).Return(nil, nil, nil) | ||
| var reviewBody = "" | ||
| var body *string | ||
| body = &reviewBody |
There was a problem hiding this comment.
Just inline &"" below where you use body? Or if that's not possible, just inline &reviewBody? Similarly below and in the other test method.
| var reviewBody = "" | ||
| var body *string | ||
| body = &reviewBody | ||
| var reviewEvent = "APPROVE" |
There was a problem hiding this comment.
Can you use the constant from the ghutil package here and in the other test method instead of copying it here?
| if labelsUpdatedForNonCompliance { | ||
| addComment(pullRequestNonComplianceReason) | ||
| } | ||
| addReview(pullRequestNonComplianceReason, ReviewRequestChanges) |
There was a problem hiding this comment.
Another thing I just realized: this is a big change of behavior for crbot, so it would be nice to control this with a command-line flag, and to be able to keep the old behavior (adding labels) as an option as well.
In fact, it would probably be safest to keep the old behavior as default, and make the new behavior something users can enable with a command-line flag, so that folks can try it out, without being suddenly surprised with a new behavior when they sync + rebuild their binaries.
|
@mbrukman Thanks for the feedback! Sorry it took me a while to get back to you. |
3 participants
For this issue: #14
When PR is not compliant, instead of just leaving a comment, the bot would leave a "REQUEST_CHANGES" review with a message of the non-compliance.