snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3707712409
##########
.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:
it still doesn't answer why we need to look at permissions here.
My impression was: having a very dummy method: get all coathors + author,
get all reviewers, find the diff, if diff == 0 then no label should be put.
Why are we looking at permissions, `APPROVE` and something else?
--
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]
