OpenSSL Communities

Proposal: a merge queue for openssl repository

Dmitry Misharov PublicSeen by 39

Nothing ever builds the commit we actually land. ghmerge rebases a PR onto current master on the committer's laptop and pushes it. The PR's CI tested a different commit, against a master that may be days old.

I propose a bot does that rebase, CI runs on the result, and master moves only if it is green. One PR at a time. A failing PR leaves the queue with a comment; master never sees it.

Worth knowing:

- Not a new CI step. At two approvals the bot already runs eleven heavy workflows. The queue runs the same set against the rebased commit and makes it binding.

- Backports become real PRs. Three quarters of release-branch commits today were cherry-picked by hand and never reviewed against that branch. The bot would open the PR.

- Review rules do not change. Two approvals, 24-hour wait, emergency path: all as they are.

- Cost: about 13% more CI. It does not fix flaky tests.

I propose to run a 4 to 6 week shadow pilot: the queue builds and tests the rebased commit and reports, while everyone merges exactly as today. Nothing lands automatically. We would then have real numbers instead of my estimates.

Full document attached. A note on venue: we have no place for proposals like this. Library designs go to `doc/designs`, policies to the policy repos, infrastructure and process proposals nowhere. Separate thread about that.

UPDATE: I created a PR to discuss this here https://github.com/openssl/project/pull/2063

Item removed

Tomas Mraz

A few comments and questions:

So the bot will apply the `addrev` annotations and push to the primary GHE repo if CI is green?

What about the flaky tests? IMO there must be a step where non-green CI is allowed to be manually overridden otherwise this process will not be workable.

Also, I'd say that we do not actually need the automated extra CI run in the PR - we should just do this run on merge. There is no need to duplicate that.

As for the cherry-picks - the PRs for cherry-picks must NOT require reapprovals if the cherry-pick is clean. Otherwise we are putting an extra unnecessary review burden on ourselves. And we cannot cope with the review queue already.

Furthermore there has to be a way to manually fix things if the automation does not get things right.

Richard Levitte (individual)

Re autosquashable commits (fixup! / squash! / amend!), I hope you're aware that they can cause conflicts, since they do infer a rearrangement of the commits.

As a matter of fact, I've started to autosquash and force-push my branches instead, to always keep the set of commits clean. Github's force-pushed link usually show the changes just fine; just don't push a rebase + fixes in one push. (this is, BTW, an expected workflow for someone that uses jj)

Richard Levitte (individual)

Also, I see the document assume that everyone use ghmerge. I don't, and would like to know if there are changes when pushing for a merge that I should be aware of.

Uri Blumenthal

Yes, this makes sense. I support this proposal.

Todd Short

I don't use ghmerge either, but I do build and test before pushing. So saying that "nothing" ever builds the commits that land is a misnomer. That being said, I know why we have the enterprise GitHub server, but it doesn't feel "open" or up-front. When a PR merges, GitHub indicates that it's been closed, rather than merged, which just seems "weird", and possibly confusing to new contributors.

I would prefer we use GitHub merge processes, but that needs to be weighed against the embargoed CVE fix process.

Richard Levitte (individual)

@toddshort, the github way would be to push to the PR branch after autosquash + addrev, and then rebase and push to the base branch. Github detects that and automatically marks the PR "merged".

I did that a few times in the past, but got some pushback from folks who wanted to continue to see the PRs in its "original" (pre-addrev, pre-autosquash) form. Me, I'm pretty 🤷 on the topic...

Richard Levitte (individual)

That being said, I don't really care about github data as much as I care about git history. In the end, it's the latter that's most durable.

Tomas Mraz

@Richard Levitte (individual) The biggest problem with pushing rebase + autosquash + addrev to the PR being merged is that the PR approvals get lost this way.

Tomas Mraz

@Todd Short I do not think using GitHub merge process is something we can adopt unless we give up on having the primary repository in the GitHub Enterprise instance. Furthermore there needs to be the commit annotation process which means that you could NOT use the GitHub merge buttons anyway. And losing the commit annotations on merge is something I am very strongly against.

Todd Short

@Tomas Mraz There are ways to maintain a linear history and have the PR merged with the PR number in the headline. It would then be an "extra lookup" to see who actually approved. So, there is no information lost, it's just not in the commit description proper. It would be interesting if there was a way to have those annotations automatically added when github merges the PR.

Todd Short

@Tomas Mraz Just because I have a preference, doesn't mean that anyone else has to have the same preference.

Tomas Mraz

@Todd Short Depending on GitHub to retrieve such information as the reviewers of a commit is really something we do not want to do. This needs to be stored in git history itself.

Paul Dale

By default ghmerge does do a build before it will let you push. You need to pass the --nobuild command line option to suppress this. This isn't a full CI run, but it does catch egregious problems.

On the other hand, pick-to-branch doesn't do a build, so the issue is real here. It does add the cherry picked from lines at least. I didn't know about ghmerge --cherry-pick before today but it seems like it's less useful although easier.