spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3709326980
##########
.github/workflows/community-review.sh:
##########
@@ -243,41 +257,72 @@ process_target_branch_label() {
# 'community-reviewed' is set.
# If one of the labels is set the other is unset.
#
+# Reviews written by someone who worked on the PR do not count towards the
community
+# review tally - neither the author's own reviews, nor those of a co-author. A
request
+# for changes is still honoured, whoever it came from.
+#
# Arguments:
# $1 - GitHub API token for authentication
# $2 - PR number
# $3 - PR reviews
+# $4 - PR author login (excluded from the review tally)
+# $5 - Labels already on the PR
# =============================================================================
process_pr_reviews() {
local token="${1?missing token}"
local pr_number="${2?missing pr number}"
local pr_reviews="${3?missing pr reviews}"
+ local pr_author="${4?missing pr author}"
+ local existing_labels="${5-}"
local communityApproves=0
local requestForChanges=0
local committerApproves=0
local communityReviews=0
local push_permission
+ local pr_coauthors
# replace spaces with new lines so the loop will work
pr_reviews=$(echo "$pr_reviews" | tr ' ' '\n')
# remove unnecessary double quotes
pr_reviews="${pr_reviews//\"/}"
- while IFS=, read -r user state time
+ # A co-author check can only lower the tally, so it can never win the PR a
label it does not
+ # already have. The approve count is a loose upper bound - it only errs
towards checking.
+ local possible_approves
+ possible_approves=$(grep -c ",APPROVED," <<< "$pr_reviews") ||
possible_approves=0
+ local coauthor_lookup_needed=true
+ if [[ "$existing_labels" =~
(^|[[:space:]])"$COMMUNITY_REVIEW_LABEL"($|[[:space:]]) ]] && [[
$possible_approves -lt 2 ]]; then
+ coauthor_lookup_needed=false
+ fi
+
+ while IFS=, read -r user state time first_review_time
do
- printf "%-15s | %-20s | %-20s - checking user permissions..." "$user"
"$state" "$time"
- push_permission=$(call_github_get_user_push_permission "$token" "$user")
|| exit
- printf "%s\n" "$push_permission"
-
- #see if the user has read role
- if [[ "$push_permission" == "true" ]]; then
- if [[ "$state" == "APPROVED" ]]; then
- ((++committerApproves))
- fi
+ # A PR author commenting on their own PR is not a community review
+ if [[ "$user" == "$pr_author" ]]; then
+ printf "%-15s | %-20s | %-20s - skipping PR author self-review\n"
"$user" "$state" "$time"
else
- ((++communityReviews))
- if [[ "$state" == "APPROVED" ]]; then
- ((++communityApproves))
+ printf "%-15s | %-20s | %-20s - checking user permissions..." "$user"
"$state" "$time"
+ push_permission=$(call_github_get_user_push_permission "$token" "$user")
|| exit
+ printf "%s\n" "$push_permission"
+
+ #see if the user has read role
+ if [[ "$push_permission" == "true" ]]; then
+ if [[ "$state" == "APPROVED" ]]; then
+ ((++committerApproves))
+ fi
+ else
+ # only worth paying for the co-author lookup once a review would
otherwise be counted
+ if [[ "$coauthor_lookup_needed" == "true" && -z "${pr_coauthors+set}"
]]; then
+ pr_coauthors="$(get_pr_coauthors "$token" "$pr_number")" || exit
+ fi
+ if is_coauthor_before_review "$user" "$first_review_time"
"${pr_coauthors-}"; then
+ printf "%-15s | %-20s | %-20s - skipping PR co-author self-review\n"
"$user" "$state" "$time"
+ else
+ ((++communityReviews))
+ if [[ "$state" == "APPROVED" ]]; then
+ ((++communityApproves))
+ fi
+ fi
Review Comment:
I think there is some confusion here, If you are talking about this block
```
printf "%-15s | %-20s | %-20s - checking user permissions..." "$user"
"$state" "$time"
push_permission=$(call_github_get_user_push_permission "$token"
"$user") || exit
printf "%s\n" "$push_permission"
#see if the user has read role
if [[ "$push_permission" == "true" ]]; then
if [[ "$state" == "APPROVED" ]]; then
((++committerApproves))
fi
```
This is just being rearranged in this PR. Its already there in the original
code. They are just being indented.
The only part I have added in to check the coauthor_lookup_needed and
is_coauthor_before_review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
