RFR: 1254: Add a checkbox for CSR requirement

[Guoxiong Li]

On Mon, 29 Nov 2021 15:25:31 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch adds a checkbox for CSR requirement in the `Progress` and adds the corresponding test cases.
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 176:
> 
>> 174:             ret.put("Change requires a CSR request to be approved", false);
>> 175:         } else {
>> 176:             ret.put("Change doesn't require a CSR request or the CSR request has been approved", true);
> 
> I don't think we should show anything here if a CSR was never requested. The difference between "CSR was never requested" and "CSR is approved" isn't really tracked currently, so that would be tricky. I'm guessing this is a big reason why the original implementation didn't add checkboxes, but only a blocker message when active. For this to work we will need to add some state that tracks if a CSR is currently required. We basically have 3 states:
> 
> 1. No CSR needed
> 2. CSR needed and not yet approved
> 3. CSR needed and approved
> 
> Every PR starts in state 1. To get to state 2, you can either issue the /csr command, or with SKARA-1071 file a CSR and let Skara find it. To get to state 3, the CSR must be approved. At any time, the command "/csr unneeded" can take you back to state 1.
> 
> This could be implemented with another label, or it could rely on special comments with markers. In this case, I think we want to make it visible to users, so labels probably make more sense. For backwards compatibility, to keep the current meaning of the "csr" label, the new label would need to be something like "csr-approved" and be added at the same time the csr label is removed when moving to state 3.

@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.

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

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