[infra] Commit automation

[gkelly]

Right now we don't have any way beyond a committer clicking the big green button to get a change landed. There are occasions where a change author will receive approval but not yet want to land a change, so having a committer always merge a change upon approval doesn't seem like the preferred flow. Do we go full-bors? Have a protocol of having authors just say "Commit please!" when it's ready might be the easier approach...

[eunchan]

GitHub has "Draft PR" feature when create a PR. Unfortunately, after normal PR is created, no way to change it to Draft. Still this is useful, as it can go through the review process but no Big Green Button is shown :)

[asb]

I don't think automation for merging is the right path: A model where patch authors commit their own changes has plenty of advantages, but it does mean that we either need to add a way to mechanically enforce cases where reviewers say "Looks good to me, but please wait for X before merging" or rely on patch submitters to follow such instructions. I think it would work fine most of the time, but given the security sensitive nature of the project we're typically making slightly different choices in areas like that to some other open source projects.

We should definitely document that patch submitters should state explicitly when they feel their submission is ready for merging from their perspective. We can also encourage pinging after a certain period of no response, and consider what we want to do to catch submissions which are waiting someone to merge them (either someone scans through periodically, or we have tooling to help with this).

[gkelly]

It'd be nice if we could use a label like "Status:Commit-Ready" or something, but I'm not sure what the GitHub permission model looks like around editing labels. We could add a webhook that would turn messages like "Ready to commit!" into "Status:Commit-Ready" labels, which would make it easier for committers to look for changes that are ready to go.

[asb]

Permissions are summarised here. Someone with only read permission can't add labels unfortunately. Having the label added might be a bit useful, but I wonder if it would be confusing project-specific process for contributors, and we might do a better job of helping out contributors by checking for approved requests that haven't actually been merged. Ideally the person doing the merging will be the committer who actually reviewed it, rather than another committer who may lack knowledge in that area.

[gkelly]

You're right. I think I'm every-problem's-a-nailing this. So long as committers doing reviews communicate with the reviewee that they'll merge their change when they're ready, and we follow up on reviews that are approved and unmerged, then I think that's probably more than enough to keep reviews moving and get changes merged.

[gkelly]

One thing that has happened a few times recently is that a change will get merged (rebased and merged) with CI results that are out of date. If we used bors or something similar that would guarantee that no change could be merged that didn't pass CI when rebased on top of the current branch...

[gkelly]

bors would also help in situations where a change looks good and a committer wants to land it but instead of being able to commit it at that point has to come back later when the pipelines run has finished.

[imphil]

Thanks for bringing this up @gkelly. I'm a bit undecided about bors. The main problem of this discussion is that bors solves multiple problems at the same time, not all of which are equally applicable to our use case. Here's my thinking at this point.

Pro:

1. I see the value bors adds when two changes conflict. That happened to us in the past, but probably not more than a handful of times? My gut feeling could be off, so please correct me.
2. As mentioned in [infra] Commit automation #976 (comment), bors solves the CI latency problem. I see the problem there. At the same time, our current CI runtime is around 20 min + queuing. Having to wait 20 min after a push seems OK to me, especially since our CI system sends out emails after passing runs which serve as reminder to press the "merge" button. This becomes more of an issue as our CI latency increases, and currently the main reason for increased latency is queuing. That's a problem we need to reduce, see below (we can't fully resolve without unlimited resources).

Con:

1. I see the "community cost" of tooling that modifies the "normal" way of how GitHub works. In particular, I find discussions in PRs already confusing and unstructured enough. Using comments as STDIN and STDOUT for a bot further lowers the signal-to-noise ratio.
2. I don't like the auto-commit functionality after a LGTM review is given. I often do "LGTM with comments" for contributors I trust. This is an essential productivity tool for me and the project, especially in an environment with people in different time zones. (I think bors has a delegate-reviews-to feature that could be used for that?)

I see three problems in our current setup which bors doesn't solve:

• The most recent problems we had with testing were around flaky tests. Bors doesn't help there.
• We have overly long CI queues at some times of the day due to not enough runners for some jobs. That's a problem that bors will make worse by doing more CI runs. The solution to that is adding more runners, and that's something we're working on.
• We don't have enough committers. I had to "LGTM for process reasons" PRs in the past which got a review from a trusted and qualified reviewer (e.g. Chris Gori or Sam) who doesn't have commit rights. AFAIK the discussion to improve this situation has started in one of the committees.

So: I'm personally not convinced the downsides of bors are outweighing the benefits. I might be wrong, so the best way to figure that out is to try it. Assuming others are OK with that, I'm happy to work with you or somebody else to integrate bors and see if it does the trick for us.

[gkelly]

Thanks for your response, @imphil, my responses inline.

Thanks for bringing this up @gkelly. I'm a bit undecided about bors. The main problem of this discussion is that bors solves multiple problems at the same time, not all of which are equally applicable to our use case. Here's my thinking at this point.

Pro:

1. I see the value bors adds when two changes conflict. That happened to us in the past, but probably not more than a handful of times? My gut feeling could be off, so please correct me.
2. As mentioned in #976 (comment), bors solves the CI latency problem. I see the problem there. At the same time, our current CI runtime is around 20 min + queuing. Having to wait 20 min after a push seems OK to me, especially since our CI system sends out emails after passing runs which serve as reminder to press the "merge" button. This becomes more of an issue as our CI latency increases, and currently the main reason for increased latency is queuing. That's a problem we need to reduce, see below (we can't fully resolve without unlimited resources).

Con:

1. I see the "community cost" of tooling that modifies the "normal" way of how GitHub works. In particular, I find discussions in PRs already confusing and unstructured enough. Using comments as STDIN and STDOUT for a bot further lowers the signal-to-noise ratio.

It does add noise, but if you look at a fairly representative PR using bors it's all legible. Commands to bors are done via @bors <command>, and can appear in other comments. Responses from bors are just comments on the PR. I think calling the interaction style with bors just STDIN/STDOUT isn't quite accurate.

2. I don't like the auto-commit functionality after a LGTM review is given. I often do "LGTM with comments" for contributors I trust. This is an essential productivity tool for me and the project, especially in an environment with people in different time zones. (I think bors has a delegate-reviews-to feature that could be used for that?)

That's correct, bors d+ allows a committer to delegate to the PR author. I think this is one of the largest wins we could get, in that it will resolve a lot of the "reviewer needs to be online" issues that I've seen.

I see three problems in our current setup which bors doesn't solve:

• The most recent problems we had with testing were around flaky tests. Bors doesn't help there.

That's correct.

• We have overly long CI queues at some times of the day due to not enough runners for some jobs. That's a problem that bors will make worse by doing more CI runs. The solution to that is adding more runners, and that's something we're working on.

Bors can perform rollup commits, where it will attempt to batch commits together for testing and merging. That could reduce the load on the CI machines in a fairly major way, as many smaller commits can get batched together.

• We don't have enough committers. I had to "LGTM for process reasons" PRs in the past which got a review from a trusted and qualified reviewer (e.g. Chris Gori or Sam) who doesn't have commit rights. AFAIK the discussion to improve this situation has started in one of the committees.

Agreed.

So: I'm personally not convinced the downsides of bors are outweighing the benefits. I might be wrong, so the best way to figure that out is to try it. Assuming others are OK with that, I'm happy to work with you or somebody else to integrate bors and see if it does the trick for us.

I believe that a trial period is worthwhile. I believe that we shouldn't force people to use it, but that we can make it available and encourage people to try it out to see if it addresses some of the issues.

[imphil]

@gkelly Since we had a fair share of master-breaking commits in recent times, I think we've how reached the point where bors can provide value to OpenTitan. Were you able to look into that topic more deeply since we last talked about it?

[rswarbrick]

I'm not sure this issue is worth keeping open. In particular, I think we've sort of agreed that the current flow works well except for the merge-skew problem. But I'm not sure this is the best starting point for an issue tracking that.

@moidx: I think you're the only contributor to this issue who's left on the project(!!). Do you think there's value keeping this open?

[moidx]

Closing as obsolete.