RFR: 1254: Add a checkbox for CSR requirement

[Guoxiong Li]

On Mon, 29 Nov 2021 18:03:05 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> @erikj79  The three states you mentioned above is clear. From your comment, I conclude the following things:
>> 
>> 1. No CSR needed: the PR doesn't have `csr` and `csr-approved` labels.
>> 2. CSR needed and not yet approved: the PR has `csr` label and doesn't have `csr-approved` label.
>> 3. CSR needed and approved: the PR has `csr-approved` label and doesn't have `csr` label.
>> 
>> And the `csr` label and `csr-approved` label can't appear at the same time.
>> 
>> But currently, the CSRBot and CSRCommand have the following logic:
>> 
>> 1. No CSR needed: the PR doesn't have `csr` label and the csr issue doesn't exist or has been withdrawn.
>> 2. CSR needed and not yet approved: the PR has `csr` label.
>> 3. CSR needed and approved: the PR doesn't have `csr` label and the csr issue has been Approved.
>> 
>> If we obey the new logic and create the new label `csr-approved`, some checks can be achieved easily. But the logic change may cause the unexpected regression. As you can see the discussion in the [PR-1245](https://github.com/openjdk/skara/pull/1245) of the SKARA-1071, both the authors of the patch and the reviewers need to be very careful to avoid the misunderstanding and avoid making a mistake.
>> 
>> So I would like to obey the origin logic. In this patch, I can check the csr link and issue to identify the state 1 and state 3 which is like the CSRBot and CSRCommand. So we don't need to add another label such as `csr-approved` to solve this problem. What is your opinion about this?
>> 
>> ---
>> And a related thing:
>> There are many places which need to get the csr issue from the original issue. We should provide a common method to solve this common problem. So I filed [SKARA-1256](https://bugs.openjdk.java.net/browse/SKARA-1256) to solve it.
>
> I needed to think about this for a bit, but I think you are right. 
> 
> I was instinctively trying to keep the PR bot from having to query JBS for CSR status, because we currently have a label which made checking the CSR state very cheap and obvious. But on the other hand, we are fetching the issue from JBS anyway during every CheckRun, so adding this extra step isn't that expensive.

@erikj79 I re-think about this. Even though we decide to use the new logic and add the new label `csr-approved` at the end,  we shouldn't implement the feature at this patch. Because the goal of this patch is only to add a checkbox for CSR requirement.

So I advice to file a new issue to continue the discussion. The new issue title can be `Investigating to add a label 'csr-approved' to mark the CSR is approved`.

Back to this patch, I think it is good to use the old logic to implement the enhancement so that the users (OpenJDK developers) can take advantage of this patch as soon as possible.

-------------

PR: https://git.openjdk.java.net/skara/pull/1246