You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "spuru9 (via GitHub)" <gi...@apache.org> on 2026/06/06 17:00:38 UTC
[PR] [hotfix][ci] Exclude PR author's own reviews from community review labeling [flink]
spuru9 opened a new pull request, #28341:
URL: https://github.com/apache/flink/pull/28341
## What is the purpose of the change
The community review GitHub Action (`community-review.sh`) applies the `community-reviewed` label when a non-committer reviews a PR. However, it also counts the PR author's own reviews — for example, a self-comment on their own PR — as a community review. As a result, a PR can be incorrectly labeled `community-reviewed` purely because its author commented on it, without any actual community review having taken place.
This change excludes the PR author from the community-review tally so that only reviews from other community members are counted.
## Brief change log
- Fetch the PR `author.login` in the open-PRs GraphQL query
- Pass the PR author login into `process_pr_reviews`
- Skip reviews authored by the PR author when counting community reviews / approvals
## Verifying this change
This change is a trivial rework of the community-review labeling script with no production code impact.
Verified locally by running the script's functions with network calls mocked:
- A PR whose only review is by its own author → no `community-reviewed` label applied (previously the label was incorrectly applied).
- A PR that also has a review from another community member → `community-reviewed` label applied as before.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (Claude Code)
Generated-by: Claude Code
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [hotfix][ci] Exclude PR author's own reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706267674
##########
.github/workflows/community-review.sh:
##########
@@ -247,11 +253,13 @@ process_target_branch_label() {
# $1 - GitHub API token for authentication
# $2 - PR number
# $3 - PR reviews
+# $4 - PR author login (excluded from the review tally)
Review Comment:
I can take a look at that. I didn't see a co-author case happening much in PRs.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "github-actions[bot] (via GitHub) via issues" <is...@flink.apache.org>.
github-actions[bot] commented on PR #28341:
URL: https://github.com/apache/flink/pull/28341#issuecomment-6597019100
This PR is being marked as stale since it has not had any activity in the last 90 days.
If you would like to keep this PR alive, please leave a comment asking for a review.
If the PR has merge conflicts, update it with the latest from the base branch.
If you are having difficulty finding a reviewer, please reach out to the
community, contact details can be found here: https://flink.apache.org/what-is-flink/community/
If this PR is no longer valid or desired, please feel free to close it.
If no activity occurs in the next 30 days, it will be automatically closed.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706957047
##########
.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
Review Comment:
not having/pressing `approve` is never a stopper for committer
committer can push directly to GitHub, so this code is mostly confusing and useless tbh... Or did I miss anything?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
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. (See 268-276 in main)
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3707070288
##########
.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
Review Comment:
It isn't gating a merge. This function picks between community-reviewed and community-reviewed-LGTM, and LGTM needs 2 approves — so if the PR already has community-reviewed and there aren't 2 approves, neither label can change and the co-author lookup is skipped.
This just help in reducing the calls.
I have added a seperate last commit for this so can remove it if its add unnecessary complexity.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [hotfix][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706837724
##########
.github/workflows/community-review.sh:
##########
@@ -247,11 +253,13 @@ process_target_branch_label() {
# $1 - GitHub API token for authentication
# $2 - PR number
# $3 - PR reviews
+# $4 - PR author login (excluded from the review tally)
Review Comment:
Done — co-authors are now excluded too, in 321bd44.
One wrinkle worth flagging, because it changed the shape of the fix. GitHub credits a reviewer as co-author the moment the author presses *Commit suggestion*, so co-authorship is sometimes a product of the review rather than something that predates it. Across the 330 open non-draft PRs there are exactly two reviewers who are also commit co-authors, #28791 and #28793, and both are that case. On #28791:
```
07-21 16:30 0b21c6ef pushed by the PR author
07-27 06:58 raminqaf leaves five reviews carrying suggestions
07-27 16:01 2f7431ad "Update .../CatalogManager.java", committer web-flow,
Co-authored-by: Ramin Gharib
```
A flat co-author exclusion would have stripped the label from exactly the reviews it is meant to showcase. So a reviewer is skipped only when they had already authored a commit on the PR before their first review; co-authorship that shows up afterwards still counts. One known gap, written up in the description: squashing a suggestion commit into an earlier one rewinds the author date and defeats the check. Using `committedDate` instead trades it for a worse failure, since rebasing rewrites that for genuine co-authors too.
You were right that "it is rare" was not the answer — chasing it turned up that the tally was also dropping a co-author's request for changes, which is now counted whoever it came from.
The lookup needs a commits query per PR, so 25f1cf1 pays for it: PR labels now come from the open-PRs query the script already pages through rather than a REST call per PR (`call_github_get_labels_api` is gone), and the lookup is skipped where it cannot change the outcome. End to end over all 370 open PRs that is 36 GraphQL calls and no REST label calls, against roughly 4 GraphQL and 185 REST before — fewer calls than the script made before any of this.
Separately, and not touched here: the draft filter at line 124 uses `select((.node.isDraft = false))` — a jq assignment, not a comparison — so it retains every PR and drafts are processed. The log gives it away, every page reports "100 PR retained" out of 100. Happy to fix in this PR or a follow-up, whichever you prefer.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3709569172
##########
.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
Review Comment:
Have removed this, will reduce some clutter.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706947819
##########
.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
Review Comment:
why are you trying to introduce check which is going different way from Apache ?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3707039471
##########
.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:
is_coauthor_before_review skips a reviewer who wrote part of the PR before reviewing it, since they aren't reviewing it as a community member.
This is to tackle the situation in which you provide suggestion, when the author accept the suggestion you get added as coauthor, but you were a review at that point.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706937309
##########
.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:
what it is about and why do we need it?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706947819
##########
.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
Review Comment:
why are trying to introduce check which is going different way from Apache ?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [hotfix][ci] Exclude PR author's own reviews from community review labeling [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #28341:
URL: https://github.com/apache/flink/pull/28341#issuecomment-4639785620
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c78d3633c751c2f6324de284a7be165ddecf1fd3",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "c78d3633c751c2f6324de284a7be165ddecf1fd3",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c78d3633c751c2f6324de284a7be165ddecf1fd3 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706948051
##########
.github/workflows/community-review.sh:
##########
@@ -247,11 +253,13 @@ process_target_branch_label() {
# $1 - GitHub API token for authentication
# $2 - PR number
# $3 - PR reviews
+# $4 - PR author login (excluded from the review tally)
Review Comment:
Have added the co-author part as well. As this increases the number of call, have tried to optimise call.
Have intentionally kept separate commits for easy review, please squash and merge when final.
Have created a JIRA as the scope is a little bigger.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3707066262
##########
.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:
and what will change?
will it just drop the label or what?
Initially I had impression the PR is only about label handling
however it looks like it is getting a way more complicated
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [hotfix][ci] Exclude PR author's own reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706325769
##########
.github/workflows/community-review.sh:
##########
@@ -247,11 +253,13 @@ process_target_branch_label() {
# $1 - GitHub API token for authentication
# $2 - PR number
# $3 - PR reviews
+# $4 - PR author login (excluded from the review tally)
Review Comment:
>I didn't see a co-author case happening much in PRs.
!= 0%
and anyway you are touching this code
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [hotfix][ci] Exclude PR author's own reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on PR #28341:
URL: https://github.com/apache/flink/pull/28341#issuecomment-5169402486
@ferenc-csaky @snuyanzin Can you take a look.
The situation I am trying to tackle in something like in https://github.com/apache/flink/pull/28895
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [hotfix][ci] Exclude PR author's own reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706252113
##########
.github/workflows/community-review.sh:
##########
@@ -247,11 +253,13 @@ process_target_branch_label() {
# $1 - GitHub API token for authentication
# $2 - PR number
# $3 - PR reviews
+# $4 - PR author login (excluded from the review tally)
Review Comment:
what about co-authors?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
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. (See 268-276 in main, FLIP-518)
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [hotfix][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706837724
##########
.github/workflows/community-review.sh:
##########
@@ -247,11 +253,13 @@ process_target_branch_label() {
# $1 - GitHub API token for authentication
# $2 - PR number
# $3 - PR reviews
+# $4 - PR author login (excluded from the review tally)
Review Comment:
Done — co-authors are now excluded too, in 321bd44.
One wrinkle worth flagging, because it changed the shape of the fix. GitHub credits a reviewer as co-author the moment the author presses *Commit suggestion*, so co-authorship is sometimes a product of the review rather than something that predates it. Across the 330 open non-draft PRs there are exactly two reviewers who are also commit co-authors, #28791 and #28793, and both are that case. On #28791:
```
07-21 16:30 0b21c6ef pushed by the PR author
07-27 06:58 raminqaf leaves five reviews carrying suggestions
07-27 16:01 2f7431ad "Update .../CatalogManager.java", committer web-flow,
Co-authored-by: Ramin Gharib
```
A flat co-author exclusion would have stripped the label from exactly the reviews it is meant to showcase. So a reviewer is skipped only when they had already authored a commit on the PR before their first review; co-authorship that shows up afterwards still counts. One known gap, written up in the description: squashing a suggestion commit into an earlier one rewinds the author date and defeats the check. Using `committedDate` instead trades it for a worse failure, since rebasing rewrites that for genuine co-authors too.
You were right that "it is rare" was not the answer — chasing it turned up that the tally was also dropping a co-author's request for changes, which is now counted whoever it came from.
The lookup needs a commits query per PR, so 25f1cf1 pays for it: PR labels now come from the open-PRs query the script already pages through rather than a REST call per PR (`call_github_get_labels_api` is gone), and the lookup is skipped where it cannot change the outcome. End to end over all 370 open PRs that is 36 GraphQL calls and no REST label calls, against roughly 4 GraphQL and 185 REST before — fewer calls than the script made before any of this.
Separately, and not touched here: the draft filter at line 124 uses `select((.node.isDraft = false))` — a jq assignment, not a comparison — so it retains every PR and drafts are processed. The log gives it away, every page reports "100 PR retained" out of 100. Happy to fix in this PR or a follow-up, whichever you prefer.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3706957047
##########
.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
Review Comment:
not having/pressing `approve` is never a stopper for committer
committer can push directly to GitHub without any approvals, so this code is mostly confusing and useless tbh... Or did I miss anything?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40320][ci] Exclude author and co-author reviews from community review labeling [flink]
Posted by "spuru9 (via GitHub)" <gi...@apache.org>.
spuru9 commented on code in PR #28341:
URL: https://github.com/apache/flink/pull/28341#discussion_r3707100303
##########
.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 doesnt drop label. If X first add a review and then become a co-author it is community-reviewed else if he already is a co-author he cant be a reviewer. (This is specifically for the suggestion part)
Yup adding co-author adds a little complexity.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org