[Investigation] Add JEP check to the PR

Wed Apr 13 12:54:09 UTC 2022

Hi all,

Recently, SKARA developers started the work about adding the JEP check to
the PR.
The enhancement was firstly reported by Jon [1]. Then we discussed it at
the skara-dev mailing list[2-6].
Currently, we have a basic design[5] and draft patch[7] to solve the issue
and would like to get your feedback.
The design is shown below for you.

---

*The main goals of this enhancement:*

*1. Prevent accidentally premature integration*

Sometimes, the author of the JEP implementation PR integrates the JEP patch
without checking if the JEP has been targeted or not in the JBS.
The JEP-295 patch[8-11] is one of the examples. It seems like a personal
mistake but is actually an issue in the PR development flow.
The JEP PR shouldn't be integrated by the bot until the JEP has been
targeted even though the author requested integration by using the command
`/integrate`.
This enhancement will add the corresponding check, which is similar to the
CSR mechanism, to prevent accidentally premature integration.

*2. Make the JEP development flow in the PR clearer*

Currently, when we want to identify whether the patch is a JEP
implementation, we can only read the comments from the PR author or other
people
and then find the JEP 'announcement' or the JEP issue link or other clues
to confirm that it is a JEP implementation.
It is not clear and not convenient. So we need to have a mechanism to
represent it, including marking it as a JEP and displaying the JEP status.

*The basic design:*

*1. Use the command `/jep <issue-id>` and `/jep <jep-id>` to mark a PR as
JEP implementation.*

Only the author of the PR and the formal reviewers can use this command.
The <issue-id> or <jep-id> must reference an issue in the JBS and the type
of the issue must be `JEP` and the issue must have the correct field `JEP
Number`.

Some command examples are listed below:
* `/jep JDK-1234567`
* `/jep 1234567`
* `/jep jep-123`
* `/jep JEP-123`

If the verification mentioned above fails, the bot will reply to a message
with a failed reason.

If the verification passes and the JEP issue have ***not*** been targeted
(the status may be "Draft", "Submitted", "Candidate", "Proposed to Target",
"Proposed to Drop" and "Close" ***without*** "Delivered"),
the bot will perform the following steps:
- Reply a successful message like `this pull request will not be integrated
until the [<JEP-id>](<URL>)  has been targeted.`
- Add a checkbox item about jep,like `[ ] Change requires a JEP request to
be targeted`, to the `Progresses` part of the PR body. Note: the checkbox is*
**not* *selected***.
- Add the jep issue link to the `Issues` part in the PR body.
- Add a PR label named `jep` to the PR.

If the verification passes and the JEP issue have been targeted
(the status may be "Targeted", "Integrated", "Completed", "Close" ***with***
 "Delivered"),
the bot will perform the following steps:
- Reply a successful message like `the JEP for this pull request
[<JEP-id>](<URL>) , has already been targeted.`
- Add a checkbox item about jep `[x] Change requires a JEP request to be
targeted` to the `Progresses` part in the PR body. Note: the checkbox is
***selected***.
- Add the jep issue link to the `Issues` part in the PR body.
The label `jep` is not added to the PR in this situation. It follows the
`CSR` convention. I think it can be re-considered.

If the previous `<issue-id>` or `<jep-id>` is wrong, you can use this
command again to re-correct it.
Only the latest `jep` command can be finally checked and the last
`<issue-id>` or `<jep-id>` can be validated. And all the previous `jep`
commands will be ignored.

*2. Use the command `/jep unneeded` to restore the state*

Only the author of the PR and the formal reviewers can use this command.
If you have used the command `/jep <issue-id>` and `/jep
<jep-id>`  mistakenly, and actually the PR doesn't need any JEP to be
targeted.
You can use this command to restore the state.

If the verification mentioned above fails, the bot will reply to a message
with a failed reason.

If the verification passes, all the previous `/jep` commands will be
ignored, too.
And the bot will remove the corresponding checkbox item, the issue link and
the label `jep` if existing.

*3. Check the jep is targeted or not automatically*

If the status of the JEP issue changes from `"Draft", "Submitted",
"Candidate", "Proposed to Target", "Proposed to Drop" and "Close"
***without*** "Delivered"`
to `"Targeted", "Integrated", "Completed", "Close" ***with*** "Delivered"`,
the bot will select the checkbox item about jep in the `Progresses` part
and remove the `jep` label in the PR. And vice versa.

Please note: the meaning of the checkbox item `[ ] Change requires a JEP
request to be targeted` in the `Progresses` is the same as other checkbox
items,
such as `[ ]  Commit message must refer to an issue`, which means if it is
not selected, the integration will be blocked.

*As a conclusion, after this enhancement, the JEP PR author only need to
simply type the command `/jep <issue-id>|<jep-id>` in the Github PR.*
*All the other things will be done by the bot and all the other mechanisms
will be performed as before.*

---

It is nice to receive your opinion and feedback about the design and the
patch[7]. Any ideas are appreciated.

Best Regards,
-- Guoxiong

[1] https://bugs.openjdk.java.net/browse/SKARA-1096
[2]
https://mail.openjdk.java.net/pipermail/skara-dev/2021-December/005481.html
[3]
https://mail.openjdk.java.net/pipermail/skara-dev/2021-December/005482.html
[4]
https://mail.openjdk.java.net/pipermail/skara-dev/2021-December/005490.html
[5] https://mail.openjdk.java.net/pipermail/skara-dev/2022-March/005770.html
[6] https://mail.openjdk.java.net/pipermail/skara-dev/2022-March/005772.html
[7] https://github.com/openjdk/skara/pull/1297
[8] https://github.com/openjdk/jdk/pull/290
[9] https://github.com/openjdk/jdk/pull/291
[10] https://github.com/openjdk/jdk/pull/740
[11] https://github.com/openjdk/jdk/pull/769