Once a merge requests can be made, #3071752: Enable ability to open merge requests for branches in issue forks, they will eventually be ready for commit.

GitLab’s UI exists and is hopefully useful. Drupal.org’s issue UI handles credits in commit message drafting.

We should also make sure both regular merges and squash merges work well.

If this is done via the GitLab UI, something like #2270763: Automatically fix issues when commit message contains "Fixed …" keyword (powerful/smart commit messages) would remove an extra bit of work.

Issue fork drupalorg-3072047

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm created an issue. See original summary.

drumm’s picture

Status: Active » Postponed
Mixologic’s picture

We need to make sure we understand how https://docs.gitlab.com/ee/user/discussions/index.html#merge-request-rev... workflow would impact the information we bring back to drupal.org

drumm’s picture

Status: Postponed » Active

There are a few options which we might want to make configurable per-project:

For the initial enabling of merge requests, I’m setting the merge method to fast forward, to match the patch workflow. There are two other options at https://docs.gitlab.com/ee/api/projects.html#project-merge-method

The option to squash or not is chosen by the project maintainer at merge time in GitLab’s UI. The person opening the merge request can make a suggestion. Once we upgrade to GitLab 13.2, there are new options: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33827

We could also add the only_allow_merge_if_all_discussions_are_resolved option.

drumm’s picture

GitLab 13.3 added squash commit options to allow/require/etc squash merges per-project https://about.gitlab.com/releases/2020/08/22/gitlab-13-3-released/#squas...

drumm’s picture

There is not an API for setting that per-project https://gitlab.com/gitlab-org/gitlab/-/issues/238531

  • drumm committed ca0eee2 on 7.x-3.x
    Issue #3072047: Do not default to deleting source branch
    
voleger’s picture

How credits (commit message) from the issue page would be populated to the merge request form?
Also, to mimic the patch flow every merge requests must use squash merging. Can squash be set as default for the new merge requests?

drumm’s picture

Can squash be set as default for the new merge requests?

GitLab does have a setting squash merges to be the default per-project, which we could set for every project. Except that it is not available in any APIs: https://gitlab.com/gitlab-org/gitlab/-/issues/238531.

We can update every merge request as it is opened, and maybe ongoing, with https://docs.gitlab.com/ee/api/merge_requests.html#update-mr. That may be useful if we want to force merge request titles or descriptions to have some standard content, or something else like that.

I fully expect we’ll end up building our own merge button on the issue page, to be able to always use our commit message.

voleger’s picture

+1 for the way to somehow merge requests from the issue page

  • drumm committed 2333a09 on 7.x-3.x
    Issue #3072047: Default new merge requests to encourage squash merging
    
drumm’s picture

All new merge requests will now be updated to default to squash merge, if that isn’t already set.

I’ve also bulk updated all open merge requests to default to squash merge, and default to not deleting the source branch. Since we do not allow issue fork branch deletion, deleting the source branch will never work.

drumm’s picture

GitLab does not quite squash commits, which would follow the patch commit style: https://gitlab.com/gitlab-org/gitlab/-/issues/1822

GitLab can get to the same result with a fast-forward squash merge. Within GitLab, that is implemented as first rebasing. For issue forks, we do not want people overwriting each others’ commits, so we disallow force pushes, which also disallows most rebases (we can detect and allow some simple rebases).

Manually pushing a squash commit does not mark the merge request as merged. GitLab does not have a way to detect if an arbitrary commit is actually merging in changes from a merge request.

To build our own merge button on the issue queue, there are a couple options to work around this:

  • Create and push the squash commit to a new branch in the issue fork, switch the merge request to that new branch, and then merge the merge request. This would have to go through a queuing system, as it would take longer than a page load should.
  • Allow issue fork Git history to be rewritten, but make a backup by either tagging or branching the previous HEAD. This lets GitLab rebase the way it wants to work, including rebasing by non-maintainers. People would be able to overwrite each others work, but there would be a way to recover the previous work. May still require a queue, we might be able to get by without one.

I’m leaning toward the second option. This lets people rewrite history as they might expect to do, but won’t lose the previous commits. And it is a less-large workaround.

drumm’s picture

https://bitbucket.org/drupalorg-infrastructure/drupal-git-hooks/commits/... works well for allowing force pushes.

https://docs.gitlab.com/ee/api/README.html#sudo works well with the credentials that Drupal.org uses. This lets the merge be done by the project maintainer.

Putting this all together for implementation should be straightforward.

drumm’s picture

The Author radio buttons will not do anything for merging merge requests when this launches, since GitLab’s API does not have a way to set a separate merge commit author. #3177220: Remove commit author radios in the "Credit & committing" section. is the issue for resolving that UX.

voleger’s picture

How about to provide the optional way to merge request manually by building the code snipped based on the selected merge request:

git checkout <target_branch>
git merge --squash <issue_branch> 
git commit --amend -m '<issue_message>' --author="<user_name> <<user_name>@<user_id>.no-reply.drupal.org>"
git push origin <target_branch>
# Don't forget close merge request as it merged manually.

Or even what about to hangle this workaround in the separate endpoint at some drupal.org service to mimic merging request with setting proper authoring of the merge commit untill it would be possible to handle that with GitLab API?

drumm’s picture

Don't forget close merge request as it merged manually.

I tested out this workflow, and GitLab does indeed not detect that the merge request is merged. I could not find a way to mark it as merged, so closing is indeed the next best.

Or even what about to hangle this workaround in the separate endpoint at some drupal.org service to mimic merging request with setting proper authoring of the merge commit untill it would be possible to handle that with GitLab API?

In general, I hope to lean on GitLab as much as possible and keep our integration lightweight. If we did build this out, it would have the same closed end state for the merge request instead of merged, although that’s not too big of a deal.

  • drumm committed 136b814 on 7.x-3.x
    Issue #3072047: Move issue fork load and branch loading into statically-...

  • drumm committed c6d69e6 on 7.x-3.x
    Issue #3072047: Add merge button to issue queue
    
drumm’s picture

Assigned: Unassigned » drumm
Status: Active » Fixed

Maintainers now have a select menu at the bottom of the Credit & committing fieldset to merge a mergeable merge request with the templated commit message.

  • drumm committed 3b460d4 on 7.x-3.x
    Issue #3072047: Log merging initiation
    
  • drumm committed 90ba457 on 7.x-3.x
    Issue #3072047: Add target branch to merge selection
    
  • drumm committed b832dcf on 7.x-3.x
    Issue #3072047: Make sure the maintainer is a git user
    
  • drumm committed e38c1a4 on 7.x-3.x
    Issue #3072047: Make sure the maintainer has access to the issue fork...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.