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

[Zhao Song]

On Tue, 9 Apr 2024 21:03:32 GMT, Erik Duveblad <ehelin 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/CheckRun.java line 112:

> 110:                 workItem.bot.confOverrideRef());
> 111:         checkablePullRequest = new CheckablePullRequest(pr, localRepo, ignoreStaleReviews,
> 112:                 jcheckParser,

Previously, for the type of parameter `confOverridingRepository` in `CheckablePullRequest`, we didn't use `Optional<HostedRepository>`, maybe it's better to still use `HostedRepository overridingRepository` as parameter in `OverridingJCheckConfiguration`.

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

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.

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

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