You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by David Radley <da...@uk.ibm.com> on 2025/05/16 15:01:33 UTC
[DISCUSS] FLIP-518: Introduce a community review process
Hi,
I have amended the design slightly for Flip-518 [1] in 2 respects
* I intend the set the COMMUNITY-REVIEW label for all types for reviews (not just approves) , as negative and comment reviews should be tracked as they are often more valuable than approvals.
* I have changed the Flip to indicate there will only ever be a maximum of one community review related label. I do not want a PR swamped with these new labels.
I will proceed to implement this. Let me know if you have any concerns with these amendments.
Kind regards, David.
[1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-518%3A+Introduce+a+community+review+process
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: Building C, IBM Hursley Office, Hursley Park Road, Winchester, Hampshire SO21 2JN
FW: [DISCUSS] FLIP-518: Introduce a community review process
Posted by David Radley <da...@uk.ibm.com>.
Hi,
In addition to the proposal below , another alternative would be to create a Github app using probot [1] – similar to boring-cyborg [2] that we already use. I am going to prototype this approach, as it seems simpler than the shared git action cache approach,
Feedback welcome,
Kind regards, David.
[1] https://github.com/probot/probot
[2] https://github.com/kaxil/boring-cyborg
From: David Radley <da...@uk.ibm.com>
Date: Monday, 19 May 2025 at 14:05
To: dev@flink.apache.org <de...@flink.apache.org>
Cc: Robert Metzger <rm...@apache.org>, Pedro Mázala <no...@github.com>, Sebastien Pereira <SP...@fr.ibm.com>, Yuepeng Pan <pa...@apache.org>
Subject: [EXTERNAL] FW: [DISCUSS] FLIP-518: Introduce a community review process
Hi,
I have been playing with git actions for FLIP-518 and have found the following.
pull_request_review
Github actions will always get a read only Github tokens when driven from a fork (our PRs come from forks) so cannot add labels or look at the collaborator role to see if they are a committer.
pull_request_target
Can get write tokens but only for main / master branches. In our case with a PR from a forked repo PR – we are not driven.
These realisations came to me when I started a discussion on the gitscript repo [2].
It therefore looks like there is not straight forward way to add labels or query the role of a collaborator from github actions that are triggered from on: specified in the git action yaml. I think this restriction is mostly around add label needing write permission which can make the git action insecure, when coming from forks with unmerged code we do not trust yet.
A periodically running workflow similar to the stale action [1] could be useful. The difference between this and the stale gitaction is that the stale git action keys off when a PR is updated. PR reviews do not update the PR.
The new proposal is to have 2 github actions using a shared cache:
1. A github action with read permission that specifies on: for pull_request_review. It then caches information. Including the time of the review and the pr. The exact content would need to be determined view prototyping
2. A second github action with write access would read the cache periodically and manage the labels appropriately.
Before I do any more work on this , I wanted to confirm with the community, if I have missed anything and we are ok with this approach.
Other notes on this approach
* The above approach provides separation between the read and write operations – through a defined structure in the cache, so should reduce the risk of attack.
* The name of the cache needs to include the git target branch name.
* Currently we have a 10Gb cache for the repo [3]. It would be good to have a view on whether this new use of caching means we should increase this limit.
* In time we could add other use cases using this pattern as required.
If there is a support for this proposal , I will write it up in more detail in the Flip,
Kind regards, David.
[1] https://github.com/actions/stale
[2] https://github.com/actions/github-script/discussions/598
[3] https://github.com/apache/flink/actions/caches
From: David Radley <da...@uk.ibm.com>
Date: Friday, 16 May 2025 at 16:04
To: dev <de...@flink.apache.org>
Cc: Robert Metzger <rm...@apache.org>, Pedro Mázala <no...@github.com>, Sebastien Pereira <SP...@fr.ibm.com>, Ferenc Csaky (Jira) <ji...@apache.org>, Yuepeng Pan <pa...@apache.org>
Subject: [EXTERNAL] [DISCUSS] FLIP-518: Introduce a community review process
Hi,
I have amended the design slightly for Flip-518 [1] in 2 respects
* I intend the set the COMMUNITY-REVIEW label for all types for reviews (not just approves) , as negative and comment reviews should be tracked as they are often more valuable than approvals.
* I have changed the Flip to indicate there will only ever be a maximum of one community review related label. I do not want a PR swamped with these new labels.
I will proceed to implement this. Let me know if you have any concerns with these amendments.
Kind regards, David.
[1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-518%3A+Introduce+a+community+review+process
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: Building C, IBM Hursley Office, Hursley Park Road, Winchester, Hampshire SO21 2JN
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: Building C, IBM Hursley Office, Hursley Park Road, Winchester, Hampshire SO21 2JN
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: Building C, IBM Hursley Office, Hursley Park Road, Winchester, Hampshire SO21 2JN
FW: [DISCUSS] FLIP-518: Introduce a community review process
Posted by David Radley <da...@uk.ibm.com>.
Hi,
I have been playing with git actions for FLIP-518 and have found the following.
pull_request_review
Github actions will always get a read only Github tokens when driven from a fork (our PRs come from forks) so cannot add labels or look at the collaborator role to see if they are a committer.
pull_request_target
Can get write tokens but only for main / master branches. In our case with a PR from a forked repo PR – we are not driven.
These realisations came to me when I started a discussion on the gitscript repo [2].
It therefore looks like there is not straight forward way to add labels or query the role of a collaborator from github actions that are triggered from on: specified in the git action yaml. I think this restriction is mostly around add label needing write permission which can make the git action insecure, when coming from forks with unmerged code we do not trust yet.
A periodically running workflow similar to the stale action [1] could be useful. The difference between this and the stale gitaction is that the stale git action keys off when a PR is updated. PR reviews do not update the PR.
The new proposal is to have 2 github actions using a shared cache:
1. A github action with read permission that specifies on: for pull_request_review. It then caches information. Including the time of the review and the pr. The exact content would need to be determined view prototyping
2. A second github action with write access would read the cache periodically and manage the labels appropriately.
Before I do any more work on this , I wanted to confirm with the community, if I have missed anything and we are ok with this approach.
Other notes on this approach
* The above approach provides separation between the read and write operations – through a defined structure in the cache, so should reduce the risk of attack.
* The name of the cache needs to include the git target branch name.
* Currently we have a 10Gb cache for the repo [3]. It would be good to have a view on whether this new use of caching means we should increase this limit.
* In time we could add other use cases using this pattern as required.
If there is a support for this proposal , I will write it up in more detail in the Flip,
Kind regards, David.
[1] https://github.com/actions/stale
[2] https://github.com/actions/github-script/discussions/598
[3] https://github.com/apache/flink/actions/caches
From: David Radley <da...@uk.ibm.com>
Date: Friday, 16 May 2025 at 16:04
To: dev <de...@flink.apache.org>
Cc: Robert Metzger <rm...@apache.org>, Pedro Mázala <no...@github.com>, Sebastien Pereira <SP...@fr.ibm.com>, Ferenc Csaky (Jira) <ji...@apache.org>, Yuepeng Pan <pa...@apache.org>
Subject: [EXTERNAL] [DISCUSS] FLIP-518: Introduce a community review process
Hi,
I have amended the design slightly for Flip-518 [1] in 2 respects
* I intend the set the COMMUNITY-REVIEW label for all types for reviews (not just approves) , as negative and comment reviews should be tracked as they are often more valuable than approvals.
* I have changed the Flip to indicate there will only ever be a maximum of one community review related label. I do not want a PR swamped with these new labels.
I will proceed to implement this. Let me know if you have any concerns with these amendments.
Kind regards, David.
[1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-518%3A+Introduce+a+community+review+process
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: Building C, IBM Hursley Office, Hursley Park Road, Winchester, Hampshire SO21 2JN
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: Building C, IBM Hursley Office, Hursley Park Road, Winchester, Hampshire SO21 2JN