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.
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.
- Use an env var/global configuration
- Use the toml set in the first Patch
- 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.
1 Like
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).
1 Like
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.
-
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.
-
Have Bors change the title of a PR from "$title" to "[Merged by Bors] - $title".
-
Have the commit title be the name of the PR, and the commit message be the PR description.
-
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.
2 Likes
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.
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
Everyone else seems okay with this as-is, so this RFC is entering Final Comment Period. Disposition: accept.
I'd really like you, @MarkRobinson, to fix the license boilerplate before this RFC is accepted. I don't want to be too picky, but it is kind of important that we get everything done by the book.
1 Like
@notriddle Sure, I'll change the license back to the default. In the boiler-plate I thought it was an option (choose A or B).
2 Likes
Yeah, sorry about that. It is an option, but it's an option for the recipient of the RFC, not one for the author. It's a dual license.
1 Like
This RFC has been approved, and added to the archive at https://bors.tech/rfcs/0349-use-a-squash-merge-in-bors.html