Use a squash merge in Bors

I don't consider that an issue (or at least not once which should limit tooling, it should be fixed in and by github, and should be possible since they're clearly messing with that flag when using the rebase or squash modes on the PR merge button / API) but yes.

Here's what we could do: Close the pull request, but edit the title to start with some text (customizable, with default value [Merged by Bors]) to indicate that the pull request wasn't really closed but was merged by Bors.

That way we don't have to wait for the GitHub team to fix this behavior.

Why not use the PR body as commit body instead? If the contents of the individual commit message are useful for some reason, they can be copied back in the PR description.

I was aiming to copy what github does. I will happily agree that the PR body is a much better commit message than the individual commits.

1 Like

DilumAluthge

Use the Github merge API w/ "squash" option enabled

@MarkRobinson Can you elaborate on what you mean by this?

Sure. There is a Github API that allows you to invoke the PR merge functionality. You can trigger it with the 'squash-and-merge' functionality by setting the mode to 'squash' when you invoke it.

Woah! That's really cool. Thanks for the tip.

I'm fine with accepting this RFC, as long as you add the alternative approaches that people are bringing up to the Alternatives section, and you verify that the code that ends up in master will always be identical to the code that gets tested in staging.

The problem is that I'm not sure if merging is actually deterministic, and even if it is the GitHub Data API might merge differently than the GitHub PR API.

If you have to mark the PR's as closed, we can live with it, even though I'd rather ensure they come up merged.

As far as I can tell, if you "merge" a pull request using GitHub's Pull Request API, you add new commits to master. In other words, the commits that you add to master will not be the same commits that were tested in staging.

In my opinion, one of the most important features of Bors is that master is only ever fast-forwarded to the same exact commit that was tested in staging.

This is my concern: How can we ensure that the code in master is identical to the code in staging? Currently, Bors ensures this by only fast-forwarding master to the same commit that passed in staging. But since the Pull Request API is creating new commits and adding them to master, we can't do that.

Could we maybe compare the tree hash of the final code in master versus the code in staging? Certainly if the tree hashes are identical, then the code is identical. But if the tree hashes don't match, does that mean that the code is definitely different? Or could you have different tree hashes for the same code content?

The only other way to ensure the code is identical would be to copy the code locally and compare it ourselves, which will require a lot of disk space and will never be feasible for the public Bors instance.

:-1: Because of these concerns, I am opposed to adding the use of GitHub's Pull Request API to Bors, and thus I would oppose this RFC in its current form

However, if you change the RFC to use @xmo-odoo's suggestion for squashing, then I wouldn't oppose this RFC. (Certainly it is unfortunate that you'd get the "Closed" instead of "Merged", but that is preferable to the concern that the commits in master are different fro the commits in staging.)

Of course, I'm not a project maintainer, so ultimately it's not my decision.

I think @xmo-odoo suggestion is a fine one. I'll change the RFC to use it. It does complicate how we would configure if the feature is enabled.

  1. Use an env var/global configuration
  2. Use the toml set in the first Patch
  3. Use the value set on master

I'd prefer (3) myself but I would rather be consistent with what bors does generally in these situations.

I prefer #2, since it's how bors normally treats its toml file.

1 Like

Incidentally, I asked github support about the ability to mark PRs as merged (to get the purple badge instead of the red "closed" one) and they replied that:

I'm not aware of any plans to provide a feature for marking a pull request as merged like you described. With that in mind, I don't expect that will happen anytime soon.

So there you are, probably not going to be a thing unless either someone has an "in" and can suggest the feature to a dev who'd be interested or possibly there's enough interest for the feature being expressed on the forum or support (or both).

Thanks for reaching out to GitHub support!

I agree, it seems unlikely that we will ever see that feature.

It's really nbd, support is responsive and reactive even if often nothing comes off of it.

I would strongly recommend anyone interested in this sort of feature ask them about it, and ping them one in a while, my understanding is it's probably the least bad way of in essence sending github feature requests (AFAIK there's no public tracker or anything, and I don't think the community forums handle feature requests outside of API) (except now that I'm thinking about it, this is an API feature request, so I should probably open a thread) (edit: done).

1 Like

I upvoted your post and added a comment of support.

Thanks for the feedback everyone. Given what people have suggested here's what I think I should change.

  1. Have Bors created Squash commits using the Git Tree API. Then apply those commits to the staging branch. Run the regular bors commit evaluation process.

  2. Have Bors change the title of a PR from "$title" to "[Merged by Bors] - $title".

  3. Have the commit title be the name of the PR, and the commit message be the PR description.

  4. We will read the first Patch in the batch during the commit process to determine if we should use the old way or the new of merging patches.

1 Like

I've updated the RFC with the comments that everyone has suggested.

2 Likes

Hey All,

I've written up an implementation. You can see it at: https://github.com/bors-ng/bors-ng/pull/718. Please add comments or suggestions. I'm very new to Elixir, let me know what needs to be updated to be idiomatic.

1 Like

So I've run into some issues with Github.

If you have any required builds which aren't triggered by the staging branch, Github will disallow the merge. This can be worked around but it can be annoying.

More concernedly, if you have any required reviewers, Github will block the merge. You can use the Bors toml file to require reviewers but Bors doesn't support the CODEOWNERS file. This means that you can't require reviews from certain people or teams.

One possible work around, is to have bors squash the PR in-place. I'm not really a fan of it because I don't think Bors should be modifying a user's branch. The other alternative is to implement CODEOWNERS support in Bors.

Any thoughts?

Neither am I.

This is probably the better option, but I don't think it's necessary for the MVP. It also might be good for people even when they use regular merge mode, improving the error messages that bors emits, so it's really a separate feature.

@notriddle

I've created an RFC to support CODEOWNERS files for Bors. Bors support for CODEOWNERS

1 Like

I've edited your RFC to have a more useful summary.

Also, before I can approved this, could you please use the normal license disclaimer? It intentionally includes both Apache 2 and CC-BY-NC-SA because the former is used for the bors codebase and stuff, while the latter is actually the default license used for content on this forum. It seems kind of silly that you edited them on both of your RFCs, when I'm pretty sure the dual license is an implied fact of the ToS for the site (which says that you're granting a CC-BY-NC-SA license) plus your license disclaimer (which says that you're granting an Apache license). But I don't like ambiguity, so let's make it explicit.

> I allow this RFC document to be modified and redistributed under the terms of the [Apache-2.0 license](http://www.apache.org/licenses/LICENSE-2.0), or the [CC-BY-NC-SA 3.0 license](http://creativecommons.org/licenses/by-nc-sa/3.0/deed.en_US), at your option.
1 Like