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

[Erik Joelsson]

On Wed, 10 Apr 2024 18:09:08 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> Hi all,
>> 
>> please review this patch that makes it possible to configure the PR bot to avoid creating a "forward backport" when integrating a pull request. A "forward backport" gets created when the issue being fixed a PR has a `fixVersion` that is less than the `fixVersion` for the repository (which is specified by `.jcheck/conf`). This patch detects this scenario and instead sets the `fixVersion` for the issue to the repository's `fixVersion` and creates a backport with the `fixVersion` of the original issue. Avoiding "forward backport"s is configurable and will only be enabled for the [jdk](https://github.com/openjdk/jdk) repository (it would be a bug to have this behaviour for update release repositories).
>> 
>> For the patch itself, there are two larger changes that are almost just moving code: the creation of the new class `OverridingJCheckConfigurationParser` and the addition of `createBackport` to `Backports` (and the removal of `JbsBackports`). `OverridingJCheckConfigurationParser` is useful for parsing a `.jcheck/conf` that can be overridden _without_ a local repository (only with a `HostedRepository`). `Backports.createBackport` is almost a clean refactoring of `JbsBackport` - no functionality has been changed.
>> 
>> There is one detail in the patch that might look weird:
>> 
>> issue.get().setProperty("fixVersions",
>>                         JSON.array().add(targetFixVersion.get().raw()));
>> 
>> In `IssueNotifier` the code is setting `fixVersions` to a value of a single JSON string. However, this value gets transformed by `JiraProject.encodeProperty`. The problem with not using an array (which the property value should be) is that tests gets confused (since `TestIssueTrackerIssue.setProperty` is *not* transforming its input). I therefore opted to use an array in the production code to make it easier to test.
>> 
>> I also added a bunch of new unit tests as well as slightly updated the integration tests for backports.
>> 
>> Thanks,
>> Erik
>
> 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.

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

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