RFR: 2226: Make it possible to avoid "forward backports"
Erik Joelsson
erikj at openjdk.org
Wed Apr 10 17:49:50 UTC 2024
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/CheckWorkItem.java line 660:
> 658: }
> 659:
> 660:
Did you intend double newlines here?
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 669:
> 667: var m = ISSUE_ID_PATTERN.matcher(pr.title());
> 668: Optional<IssueTrackerIssue> issue = m.matches() ? issueTrackerIssue(getMatchGroup(m, "id")) : Optional.empty();
> 669: if (issue.isPresent()) {
Don't we need to check that the main issue is open?
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 691:
> 689: var issueFixVersion = Backports.mainFixVersion(issue.get());
> 690: if (issueFixVersion.isPresent() &&
> 691: targetFixVersion.get().compareTo(issueFixVersion.get()) > 0) {
This check should be made before `willCreateBackport` as it's much cheaper.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java line 702:
> 700: var backport = assignees.isEmpty() ?
> 701: Backports.createBackport(issue.get(), issueFixVersion.get().raw()) :
> 702: Backports.createBackport(issue.get(), issueFixVersion.get().raw(), assignees.get(0).username());
The order of operations here risks leaving a mess if the bot is interrupted at some point. The fixVersion could be updated in the bug, but no backport created and no comment posted. On a rerun nothing would happen. The backport could be created but no comment be posted. On a rerun, no comment would be posted.
Operations like this need to be carefully crafted to guarantee that the end state is eventually reached. The simplest fix would be to create the backport first. Then we would also need to check for a backport with the same fixVersion as the main bug and only create the backport if that doesn't already exist. After that we change the fixVersion of the main bug.
That still won't guarantee that we will post the comment. For a complete solution we will need to post an interim comment with the wording "going to" added before performing any operations. We can look for this comment in addition to the other conditions to detect an incomplete existing attempt. Then when we have performed all operations, we finalize the comment by rewriting it.
We also need tests that verify that the end state is reached when the bot is presented with a PR in any of these incomplete states.
This may seem pedantic, but I'm very sure this will actually happen, more often than one might think. I went through a similar exercise with the /integrate command soon after taking over Skara because it was rather common for the bot to be interrupted mid integration.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/OverridingJCheckConfigurationParser.java line 39:
> 37: private final List<String> overridingConf;
> 38:
> 39: OverridingJCheckConfigurationParser(Repository localRepo, Optional<HostedRepository> overridingRepository,
Using Optional as a parameter isn't considered good form. I would rather have a null check in the constructor below.
Another option is for the constructors to take a PullRequestBot instance which this class can extract the override config from. All uses of this class fetch all three parameters from the PullRequestBot instance anyway.
Or if we really want to go the extra mile, the three override options always go together, and are only ever consumed by this class, so maybe a single carrier class for the config that this class can consume would make more sense?
-------------
PR Review Comment: https://git.openjdk.org/skara/pull/1630#discussion_r1559528925
PR Review Comment: https://git.openjdk.org/skara/pull/1630#discussion_r1559833695
PR Review Comment: https://git.openjdk.org/skara/pull/1630#discussion_r1559838400
PR Review Comment: https://git.openjdk.org/skara/pull/1630#discussion_r1559825360
PR Review Comment: https://git.openjdk.org/skara/pull/1630#discussion_r1559522666
More information about the skara-dev
mailing list