-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Misleading reason, why merging a PR is blocked #13655
Comments
It would be better to only add a warning "Not all requested reviewers have reviewed.", but still allow merge. As it was anyhow the writer of the PR that made the review requests. |
I think this is an expected behaviour ... and repoAdmins + requested reviwer can dismiss request to review if needed. |
@ckittl you you propose to allow a review-request-creator to dismis those requests he had maid? |
Thank you for your consideration, @6543! I think, revoking a review request is still possible, yet. I don't really bother about the pull request being blocked. If this is intended behaviour, I'm fine with it. It is just that I didn't expect this behaviour, as it is different for famous github. I would more wish for a clarification, by e.g. renaming the headline shown in the screenshot I made. |
Outdated, the code has changed a lot |
Sorry, cannot answer the following information, as I'm not hosting the Gitea instance myself. AFAIK, it doesn't play a role either.
Description
With #9592, blocking merging a pull request was enabled, if one reviewer requested changes on the code. However, with ef89e75 (regarding #10756) the checks got extended to also cover the case if one explicitly requested reviewer hasn't given any comments yet. I feel a bit confused about this in two ways.
Think of the following test case (cf. also the test repo):
The information, why merging is blocked is misleading
As far as I was able to get from your code, the reason, why a pull request merge is blocked, is determined here:
gitea/routers/repo/issue.go
Lines 1435 to 1445 in 1bb5c09
However, obviously the condition is true also, if nobody requested changes, but still one of the explicitly requested reviewers didn't answer yet. This leads to the misleading prompt, that requested changes not have been addressed, yet.
My suggestion would be to spin of
Branch#MergeBlockedByExplcitReviewRequest
fromBranch#MergeBlockedByRejectedReview
and have both conditions separated.The explicit review request override my global config
As with GitHub, I would expect, that everything is fine to be merged, as long as one reviewer has replied with approval. However, current implementation blocks the merge with no need. Despite me not finding it handy to require all requested reviews, at least it is not clear, that this is the case. Maybe a renaming of the label for review requests from "Reviewers" to "Mandatory Reviewers" would point out clearly, what it's about (cf. Screenshot).
Screenshots
The text was updated successfully, but these errors were encountered: