Conversation
ca0b91a to
4564f87
Compare
0cb863c to
bbe8ad4
Compare
bce87d3 to
dae2bdc
Compare
|
|
||
| /** | ||
| * Generates a Check Run or modifies the existing one. | ||
| * This way we can aggregate all the results from different causes into a single one |
There was a problem hiding this comment.
Does it mean that you have solved the problem of two status checks - from pull_request and from pull_request_review triggers?
There was a problem hiding this comment.
Kind of yes. you will still find two triggers if it is triggered from both events.
But now they create an extra check which contains the final result, so we only use that one as the blocker.
rzadp
left a comment
There was a problem hiding this comment.
No objections from my side.
|
@mutantcornholio @mordamax any objections or should I merge it? |
| const success = await this.validatePullRequest(config); | ||
| const reports = await this.validatePullRequest(config); | ||
|
|
||
| this.logger.info(reports.length > 0 ? "There was an error with the PR reviews." : "The PR has been successful"); |
There was a problem hiding this comment.
This log line feels off.
Judging by the code of generateCheckRunData, we can have reports > 0, while still having enough reviews to mark the PR green, if there aren't any missingReviews.
There was a problem hiding this comment.
Not really. validatePullRequest will only return reports for failed rules, but if no rule failed, it will return an empty array.
So, unless it failed, generateCheckRunData will be working with an empty array and returning a positive case.
There was a problem hiding this comment.
I guess, I got confused by it because there is two different ways to check for success/failure. One is reports.length > 0, and another is reports.reduce((a, b) => a + b.missingReviews, 0) > 0.
Also, it's a bit more clear now when the annotation on what this.validatePullRequest() returns is updated.
| const success = await this.validatePullRequest(config); | ||
| const reports = await this.validatePullRequest(config); | ||
|
|
||
| this.logger.info(reports.length > 0 ? "There was an error with the PR reviews." : "The PR has been successful"); |
There was a problem hiding this comment.
I guess, I got confused by it because there is two different ways to check for success/failure. One is reports.length > 0, and another is reports.reduce((a, b) => a + b.missingReviews, 0) > 0.
Also, it's a bit more clear now when the annotation on what this.validatePullRequest() returns is updated.
Added ability to generate a status check and the ability to overwrite the latest one.
This resolves #33 and closes #26 as it merged it into one ticket.
There are some limitations as we can not select the check suite that the check will belong (see #33 (comment)), but the name will be constant, making it validable.