1663: Mark integrated Pull Requests as properly merged in their repositories

[TheShermanTanker]

Skara currently closes integrated Pull Requests, since the actual integration is done internally and then pushed to the repository separately. This makes every integrated request always look like it was closed to an outside observer, forcing the use of a special integrated flag to distinguish between integrated and closed, or rejected and closed. On top of that, it just looks awful in the interface all around (and is also not included in the accepted Pull Request count by GitHub for the committer, which is frustrating for newer contributors to a surprising extent). In the willful absence of a reply from the GitHub team to allow marking a pull request as merged through a flag, and GitLab being unlikely to make a similar change either, we can look at implementing this as an integration feature in Skara itself instead.

Every pull has a corresponding pre-integration branch created at the time it was made, and this special branch is marked for deletion when the pull is integrated, and initially it was thought to merge the Pull Request into these corresponding pr/ branches just before their deletion but that proved to present too many challenges to be viable. Instead a related approach of enhancing the Pull Request bots with their own special integration branches can be taken, as well as providing a flexible utility method for extracting the actual branch the Pull Request was integrated into, to address the concern of losing information on what the actual Pull Request target branch was.

https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/merge_request.rb#L172

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace

Issue

• SKARA-1663: Mark integrated Pull Requests as properly merged in their repositories

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/skara.git pull/1409/head:pull/1409
$ git checkout pull/1409

Update a local copy of the PR:
$ git checkout pull/1409
$ git pull https://git.openjdk.org/skara.git pull/1409/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1409

View PR using the GUI difftool:
$ git pr show -t 1409

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/skara/pull/1409.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jwaters! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 58: Full - Incremental (52af6bb2)
• 57: Full - Incremental (21167d4a)
• 56: Full - Incremental (e748d2fb)
• 55: Full (ffbb82f9)
• 54: Full - Incremental (1dd18829)
• 53: Full - Incremental (b411ed31)
• 52: Full - Incremental (a37fb02b)
• 51: Full - Incremental (cc4407e7)
• 50: Full - Incremental (076b3489)
• 49: Full - Incremental (94772742)
• 48: Full - Incremental (9a4f3881)
• 47: Full - Incremental (4ac3afc4)
• 46: Full - Incremental (96c3f471)
• 45: Full - Incremental (4c9cb948)
• 44: Full - Incremental (211f0f16)
• 43: Full - Incremental (dd8b110c)
• 42: Full - Incremental (307f02d7)
• 41: Full - Incremental (cd02b297)
• 40: Full (f8c8d446)
• 39: Full - Incremental (e20cd3a7)
• 38: Full - Incremental (fc9aadcb)
• 37: Full (03761978)
• 36: Full - Incremental (6a7814a7)
• 35: Full - Incremental (b746948a)
• 34: Full - Incremental (104ea0e7)
• 33: Full - Incremental (1b10af17)
• 32: Full - Incremental (00ee2c43)
• 31: Full - Incremental (35b18015)
• 30: Full - Incremental (ab1ac9d5)
• 29: Full - Incremental (c9b3386b)
• 28: Full - Incremental (c38176d5)
• 27: Full - Incremental (8417bd8d)
• 26: Full - Incremental (c608c108)
• 25: Full - Incremental (4b4d23cd)
• 24: Full - Incremental (50baa250)
• 23: Full - Incremental (b931538c)
• 22: Full - Incremental (edf40542)
• 21: Full - Incremental (6bfaf50f)
• 20: Full - Incremental (3317161e)
• 19: Full - Incremental (70eb3c3f)
• 18: Full - Incremental (26eefcce)
• 17: Full - Incremental (94101112)
• 16: Full - Incremental (37b6501c)
• 15: Full - Incremental (bac2d40d)
• 14: Full - Incremental (3fcc5137)
• 13: Full - Incremental (72191afb)
• 12: Full - Incremental (a84bc918)
• 11: Full - Incremental (8f2f06ed)
• 10: Full - Incremental (dadc5d87)
• 09: Full - Incremental (eae045d6)
• 08: Full - Incremental (a5cf28c7)
• 07: Full - Incremental (d90f9042)
• 06: Full - Incremental (673e95d9)
• 05: Full - Incremental (1e79c875)
• 04: Full - Incremental (804df720)
• 03: Full - Incremental (484f220d)
• 02: Full - Incremental (ebdace0b)
• 01: Full - Incremental (8ca08323)
• 00: Full (078b4f77)

[TheShermanTanker]

I've only just realized that there are tests that require careful change whenever the integration marking code is modified. The existing integration sequence hasn't been touched whatsoever but there is new logic executed before it that I can't figure out whether the corresponding test code needs to be updated. Will add that as a task for now

[erikj79]

First of all, for a change as big as this, a bug is definitely needed. It should really be filed first and explain what and why you intend to change.

This is an interesting idea, but I'm not sure its worth the tradeoff of losing the information about which branch a PR was integrated into. We already have multi branch development happening in some repos and more of that is coming.

At first glance this also looks brittle. You need to understand that the bots are designed to be interruptible at any point in time, restarted and being able to continue. The integration logic and order is carefully crafted to function this way. As an example, assuming that the second to last target targetRef change is the one we actually integrated into could be false if someone else was changing that around the time of integration. There are a lot of corner cases that would need careful testing before this change could be accepted, and once it went live, I would expect at least some fallout.

We are currently blocked from using pr/X branches in GitLab due to SKARA-1173, so this proposed change would be blocked until that is resolved regardless.

[openjdk-notifier]

@TheShermanTanker Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

[TheShermanTanker]

First of all, for a change as big as this, a bug is definitely needed. It should really be filed first and explain what and why you intend to change.

This is an interesting idea, but I'm not sure its worth the tradeoff of losing the information about which branch a PR was integrated into. We already have multi branch development happening in some repos and more of that is coming.

At first glance this also looks brittle. You need to understand that the bots are designed to be interruptible at any point in time, restarted and being able to continue. The integration logic and order is carefully crafted to function this way. As an example, assuming that the second to last target targetRef change is the one we actually integrated into could be false if someone else was changing that around the time of integration. There are a lot of corner cases that would need careful testing before this change could be accepted, and once it went live, I would expect at least some fallout.

My mistake- I forgot to create the issue and enable the test workflows, both have now been done.

I've since reverted the changes to PreIntegrations, which should resolve the problem of concurrent changes to the targetRef in question. The only other solution I can think of though is saving the targetRef before the pr/ branch swap happens, and then immediately resetting it back to the saved instance after the merge in markIntegratedAndMerged (and if really required, also printing the branch the Pull Request was integrated into in the "pushed as commit" message). I unfortunately currently have no way of testing if GitHub even allows doing this however, and documentation doesn't really seem to mention anything about this either

[erikj79]

That's a bit of a bummer, though maybe that can be circumvented with a few instanceof GitHubPullRequests? The change is, after all, only actually implemented for GitHub

I don't understand what you mean by the change only being implemented for GitHub. We strive to keep Skara feature equivalent on GitHub and GitLab.

The pr/X branch feature is optional, so those branches are currently not present in all repositories that the PR bot operates on. This configuration is done in the notifier bot, as that is currently the only bot that needs to know about it, so the PR bot does not know if the feature is active. It would have to check for the presence of such a branch at integration time, and we would have to trust that no other branches with such names ever show up in repos where we haven't activated the pr/X feature.

I don't think the proposed strategy of using the pr/X branches to trick the forge into marking a PR as "merged" is viable for us. To me, the tradeoff between extra complications and the potential gain isn't worth it.

[TheShermanTanker]

The pr/X branch feature is optional, so those branches are currently not present in all repositories that the PR bot operates on.

My mistake, I misunderstood the original comment; I didn't realize that the PR bot had no way of telling if the feature was active on the main repos

[bridgekeeper]

@TheShermanTanker This pull request has been inactive for more than 3 weeks and will be automatically closed if another 3 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[TheShermanTanker]

An early look at how integrated Pull Requests look like after this change: Elshout/Sandbox#2

I will definitely need more time to work on this to make it run better though, currently it's a bit of a mess

[TheShermanTanker]

Improved Version: Elshout/Sandbox#1

Elshout/Sandbox#2

[altrisi]

Is there any reason why Skara doesn't use (or emulate in a way that is detected if there's no direct way/endpoint) what GitHub does when using the "Rebase and Merge" button?

That marks pull requests as merged unless it's changed recently, without making the rebase directly in the PR branch.

[TheShermanTanker]

@altrisi could you elaborate further on this "Rebase and Merge" feature for GitHub? Do you mean the usual rebasing and merging of the PR's actual sequence of commits directly into the repository, or is this part of GitHub's indirect merge logic (where the Pull Request does not need to be actually merged into the target branch for it to be marked as merged, I am aware that there are a few circumstances where GitHub will currently do that) that I haven't heard of? If it's the latter, I am pretty curious and interested

[altrisi]

I'm talking about the Rebase and merge option on the dropdown when manually merging a PR from the web UI, just wondering if there's a reason not to use that or something similar in Skara. I don't know if it's exposed to apps, or if Skara has more requirements that feature doesn't meet though.

[altrisi]

Sorry I've said it wrong, I was referring to Squash and merge this entire time, for some reason confused it with rebase.

[erikj79]

Is there any reason why Skara doesn't use (or emulate in a way that is detected if there's no direct way/endpoint) what GitHub does when using the "Rebase and Merge" button?

That marks pull requests as merged unless it's changed recently, without making the rebase directly in the PR branch.
Sorry I've said it wrong, I was referring to Squash and merge this entire time, for some reason confused it with rebase.

An important design choice in Skara was to make sure it was compatible with at least 2 different hosting providers (currently github and gitlab) so that we did not build in a hard dependency on a single provider. This means we can't lean too heavily on functionality specific to any one provider, but rather prefer to implement general solutions that can be used with both/all.

I can't say if there is anything in particular that the GitHub API of today is missing that would hinder us from using it for squash merging PRs, at least not without spending a considerable amount of time investigating. I'm pretty sure the original implementors of Skara found that the current strategy was necessary to support the OpenJDK process on both GitLab and GitHub at the time it was implemented, and I trust their judgement.

[TheShermanTanker]

I can't say if there is anything in particular that the GitHub API of today is missing that would hinder us from using it for squash merging PRs, at least not without spending a considerable amount of time investigating.

From my time experimenting with the APIs of both hosts, the only thing really missing is the ability to set the committer and author of the actual Pull Request, like we currently do in CheckablePullRequest. Otherwise quite a lot of the existing logic could (with substantial effort) be migrated to use the Pull Request API instead of the current strategy of creating a commit on our own local repositories before pushing to remote ("commit_title" and "commit_message" can already be set from the API for instance, same with GitLab). A rather extreme option to circumvent this issue if we really wanted to utilize the host's merging abilities as altrisi suggests would be giving Skara the ability to force the Pull Request itself (the branch that contains its commits in essence) to allow edits from the pr bot (both GitHub and GitLab support doing so) and pushing the commit from our own local repo into the PR instead, before then merging it through the API as if from the UI as per normal

(Side note: Doing it this way would also result in having to make messy decisions on what to do with the existing prepush comment system and the handling for the "Pushed as commit" comments, since those would no longer be accurate in such a scenario)

[openjdk]

⚠️ @TheShermanTanker This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[openjdk]

@TheShermanTanker this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout integration
git fetch https://git.openjdk.org/skara master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[bridgekeeper]

@TheShermanTanker This pull request has been inactive for more than 6 weeks and will be automatically closed if another 6 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@TheShermanTanker This pull request has been inactive for more than 12 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.