RFR: 2226: Make it possible to avoid "forward backports"

[Zhao Song]

On Wed, 10 Apr 2024 20:59:52 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 666:
>> 
>>> 664:             //
>>> 665:             // NOTE: The current implementation only works for the issue in the title, *NOT* for all issues solved by the PR
>>> 666:             if (bot.avoidForwardports()) {
>> 
>> I am not sure if we should take action with forward port at this early stage. I mean if a user had a typo in the pr title(the issue id is wrong) and this made the pr like a forward port, then Skara bot would set the fixVersion to null for the incorrect issue and create a backport. Even if the user fixes the typo in the pr title, Skara bot will not withdraw the previous action. The user will need to fix it manually.
>> I think maybe it's better to take action at least when the pr is marked as "ready".
>> 
>>> // NOTE: The current implementation only works for the issue in the title, *NOT* for all issues solved by the PR
>> 
>> And if you want, I think we can make it work for all the issues associated with this pr.
>> We can get all the issue ids via `BotUtils.parseAllIssues(prBody)`.
>> 
>> 
>> I am also wondering why don't we put the whole logic of `avoidForwardports` in `issueNotifier`, it's where the backports will be created.
>
> You bring up a good point with user mistakes having the potential to create lots of bad backports without any automatic cleanup from the bot. We could potentially handle that by doing it in the `IssueNotifier::onNewIssue` and `::onRemovedIssue`, but it would be brittle. Their current use, which is to add links in the issue, is easily revertible, but backport creation is definitely messier.
> 
> The core of the issue is that we are trying to create the backport at PR creation rather than at PR integration. This was a design decision we made a while back, before considering the implementation, as we thought it would be better from a JBS user point of view. The issues would be updated to reflect the correct fixVersion and backports earlier.
> 
> @edvbld and I had a discussion and we think that it's probably better to instead do any backport creation at PR integration (which I think means in the `issueNotifier`). However, we need to make it clear to the user that this is going to happen, so the PR bot would still need to maintain comment (likely a section in the merge ready comment) describing exactly which fixVersion would be changed and what backports would be created. I think this description could be useful for all PRs, regardless of using this feature or not, so that users know what to expect with regards to fixVersion and backport creation. To achieve that, we probably need to implement some more common logic between the two bots.

Thanks for the details! I was thinking the users might be confused when they found in forward port cases, backports are created at PR creation while they already got used to see backports created at PR integration when they are using /backport command.

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

PR Review Comment: https://git.openjdk.org/skara/pull/1630#discussion_r1560096825