GitHub Action Commit Queue

[Trott]

My thinking is let's focus on making this a GitHub Action that can be run when Jenkins is green/yellow. Maybe triggered by a Collaborator comment? It can re-run some things as GitHub Actions to make sure tests still pass on the most popular platforms before adding metadata and merging.

Who wants to be involved? We should probably add and remove people from @nodejs/commit-queue as appropriate.

Refs: nodejs/commit-queue#1

[mmarchini]

I'm in (and I agree with the approach described). Maybe we should fork nodejs/node to work on it (since we'll have to test landing things)?

[richardlau]

I'm in (and I agree with the approach described). Maybe we should fork nodejs/node to work on it (since we'll have to test landing things)?

https://github.com/nodejs/node-auto-test ?

[mmarchini]

My suggestion is we start by making an action that lands the PR (without any checks at first), because there are restrictions for PRs coming from forks, and we'll need to work around those restrictions. I can take a stab at drafting something tomorrow or Thursday.

[nschonni]

Here is one that runs off of the PR being labeled https://github.com/pascalgn/automerge-action
There are some others in Marketplace that might be used as a baseline if they don't entirely fit https://github.com/marketplace?utf8=%E2%9C%93&type=actions&query=pull+request

[mmarchini]

@nschonni thanks, that's probably a good place to start. It'll probably not fit us though since we add metadata to every commit, and usually use node-core-utils to land pull requests, but we can base our implementation on that.

pascalgn/automerge-action suffers the fork issues I mentioned above (pascalgn/automerge-action#46). More specifically, for an action make any changes (merge, commit, comment, add labels, etc.) to a Pull Request, it makes a call to the GitHub API using a GITHUB_TOKEN provided to the action. Usually that token has read and write access, but if a Pull Request is coming from a fork, the token is read-only:

The permissions for the GITHUB_TOKEN in forked repositories is read-only.

(https://help.github.com/en/actions/reference/events-that-trigger-workflows#pull-request-events-for-forked-repositories-2)

Looking at the other events available, I don't see any we could use directly, but here are some ideas:

1. triggering externally, which can be used in two different ways:
   • Create a separate interface/ncu command to trigger commit queue on a Pull Request (only slightly better than the current workflow)
   • A comment/label action to make a call to an external API, which will make a call back to GitHub to trigger a repository_dispatch event, which will run appropriate checks and merge the PR if possible (this is the UX we're aiming for, but it sounds like a more complex solution). The external API would be responsible to check that the event was triggered by a Node.js core collaborator (if we can't validate that for sure, this approach is a no-no)
2. Use the scheduler event to iterate over Pull Requests with a specific label (let's say, commit-queue) and merge them
3. Have a separate branch from master where we land things, and when we land there, GitHub triggers an Action to check, add metadata and push to master, or to revert + reopen PR if checks fail

I think 2 and 3 would have less maintenance burden, and would be easier to ensure only collaborators can land things. 3 could cause a lot of confusion though, so maybe we should try something with the scheduler? Or does anyone have other suggestions?

[jasnell]

I'd be generally +1 on this but would definitely like to see it well tested in a separate repo (not core) and accepted before we enable it here.

My chief concern is around commit squashing. Most of the time squashing is straightforward, but there are often fixups that we need to do to fix formatting, subsystem in the title, typos, munging of comments from squashed commits, etc, that often really does need to be done manually.

[mmarchini]

I'd be generally +1 on this but would definitely like to see it well tested in a separate repo (not core) and accepted before we enable it here.

+1

My chief concern is around commit squashing.

We can start forcing only single-commit PRs only

Most of the time squashing is straightforward, but there are often fixups that we need to do to fix formatting, subsystem in the title, typos, munging of comments from squashed commits, etc, that often really does need to be done manually.

I don't think a commit queue is supposed to eliminate manual landing, but rather make it easier to land common cases. It would be the collaborator's discretion to decide if they want to merge via commit queue or manually.

[jasnell]

It would be the collaborator's discretion to decide if they want to merge via commit queue or manually.

Initially yes, but as I understand it, the whole idea behind the commit queue is to remove the collaborator's discretion from the mix as much as possible. I'm definitely not against it; but while at times I start to feel like the Old Man Who Hates Everything New, I'd like to see us take our time to get it right.

[mmarchini]

I think I like the scheduler idea, it fits well with our "wait until land" policy (for example, the second collaborator who approved a PR could add the commit-queue flag, and after the 48 hours passed GH Actions will land it).

[joyeecheung]

I think it's good to have unconditional squashing in the beginning - it seems understandable to me that if you want something more complicated like landing multiple commits with fixups, you'd need to count on a human being to do the chores instead of expecting too much out of automation. If we only ever support single-commit landing in the automation, we also encourage the culture of sending different PRs for different commits if possible - but as always we could leave room for human beings to land things as they wish, instead of making it impossible to land manually.

Regarding commit message for the squashed commit - how about taking the PR message? Then the workflow would be somewhat similar to how chromium's commit queue works - before you commit, you change the only CL description which is eventually used as commit message.

[joyeecheung]

Looking at the other events available, I don't see any we could use directly, but here are some ideas

I think 3 sounds safer - it also sounds oddly like the LKGR + master strategy that chromium does, where they have sheriffs to keep an eye on the breakage and revert ASAP without caring too much about how significant the PR is. If we could also adopt a similar culture (prioritize revert + reland over waiting for a fix) that would be a better choice IMO.

[mmarchini]

I've been playing with the schedule event and while I think it would work, it would have some extra complexity as well (especially because the context GitHub Actions give us on schedule is rather incomplete).

My biggest concern with 3 is:

3 could cause a lot of confusion though

To elaborate on that, if we have two branches (let's say master and commit-queue) and we land things on commit-queue, one of two things would happen:

1. When folks open a Pull Request it would still point to master by default, or
2. The front page of https://github.com/nodejs/node would point to commit-queue instead of master.

Also, automating Revert can get quite complex.

(edit: I just double-checked, there's no way to select different branches for the front page and for default PR target)

[joyeecheung]

When folks open a Pull Request it would still point to master by default, or

Why would this be an issue if the "intend to land" branch is indeed master? It still sends the message that this is not intended to land on e.g. a backport branch. Provided that we still leaves room for people to land commits manually, it still makes sense to point to the master branch and the temporary branch is more like an implementation detail of the action.

Also, automating Revert can get quite complex.

Can the action just posts something to IRC and ask a human being to do it ASAP?

[mmarchini]

Why would this be an issue if the "intend to land" branch is indeed master?

A collaborator (or the author) would still need to change the branch manually. It works, but it's not the most ergonomic experience.

Can the action just posts something to IRC and ask a human being to do it ASAP?

It can also comment on the Pull Request and reopen it I guess. In theory it could just push a Revert commit of itself to commit-queue, I'm just worried we would eventually get too out of sync with master (regardless of the Action doing it or we doing it manually). I also think rebasing and removing commits is not an option, because if there are commits not pushed to master and we rebase/force-push, only the last commit will execute the Action.

I don't think any of the three suggestions are perfect (far from it, as they all are workarounds), so I'm totally fine trying this one out.

[joyeecheung]

A collaborator (or the author) would still need to change the branch manually. It works, but it's not the most ergonomic experience.

Can't we just keep it as an implementation detail? i.e. the action changes the branch accordingly under the hood and resets master to the the working branch periodically, to people there is just a delay (for verification) in how long the commit eventually lands on master. As a result, nobody can touch master (unless exceptions are made), even people who land the commit manually to the working branch - they will have to wait for the action to sync their landed changes to master (so technically, master becomes LKGR while that working branch is what we currently use as master)

[joyeecheung]

I also think rebasing and removing commits is not an option, because if there are commits not pushed to master and we rebase/force-push, only the last commit will execute the Action.

Is it talking about people doing rebasing/removing, or the action? I think to prioritize the branching strategy for automation we might end up with something like what's done in chromium - the master branch is strictly linear and people can't rebase or remove anything, they could only fix it up by piling revert/reland onto the branch, or just let it go if it's just some minor issues in the commit message, etc. Which seems OK to me

[mmarchini]

Can't we just keep it as an implementation detail? i.e. the action changes the branch accordingly

But the action will only be triggered after merge, in which case the commit will already be on master. Maybe we're thinking different approaches?

[joyeecheung]

@mmarchini I was thinking about something like this

commit
->
action lands it on master-tentative (or any other name)
->
action tries to verify
->
if master-tentative is broken, the action pings on IRC for a revert, and halt the landing of other PRs; either way, it closes the original PR, and sends a message about the result. The PR can be reopen or a reland could be opened separately depending on how people want to get it done.

Meanwhile, another action would also periodically reset the master branch to the last verified commit of master-tentative (so, master effectively becomes LKGR, and this branching strategy would be similar to how chromium does it). master-tentative could be modified in a non-linear way under exceptional circumstances (or maybe not..?) , while master is strictly linear.

[joyeecheung]

hmm, or maybe the action should trigger a CI (with the rebase option on) before it lands the PR and only actually perform the landing on master-tentative after it comes back green, or in the case it fails, get an explicit command to proceed from a human.

[mmarchini]

Ok, I understand your approach now, and I agree this would be great. The problem is:

action lands it on master-tentative

The collaborator needs to land it on master-tentative (change the base branch on PR and click Merge there). The Action doesn't have permission to do that.

if master-tentative is broken, the action pings on IRC for a revert, and halt the landing of other PRs; either way, it closes the original PR

When the action runs (with appropriate permissions), the PR is already closed.

The whole problem here is that GitHub Actions triggered by events in a Pull Request coming from a forked process do not have edit permission, so it can't change the base branch, close a PR, comment on a PR, or push to master. I also think it can't access secrets (which would be needed to trigger a Jenkins CI), but I'm not sure.

So either we still need to use schedule with labels/comments (because schedule runs with appropriate permissions), or we need to use the github-bot to accomplish this workflow. A compromise would be to change the base branch manually and use the merge button to land things on master-tentative.

[joyeecheung]

I see, if the action triggered by the merge can only act on the commits post-merge, it seems something out-of-band (e.g. schedule event) is indeed necessary for formatting the patches.

If nothing can be done before the commit lands on a branch (master or not), then we either have to edit one master branch post-merge frequently, or having a unformatted working branch and a formatted LKGR branch out of sync. IMO the first sounds more error-prone to me while it seems possible to work around the second if we maintain that a) the commits on the working branch and the patches on the master branch are in 1-1 correspondence b) the commits on the LKGR branch are formatted versions of the commits in the working branch; c) commits are formatted only once and only done right before they are moved from the working branch to the LKGR branch

[mmarchini]

If we're fine with an hybrid solution, we could use the github-bot to parse messages, and trigger a workflow remotely.

[mmarchini]

I've been thinking about this again recently. If we want to keep it simple, can we try this flow:

1. Collaborator adds commit queue label to PR
2. Scheduled Action will run every 5 minutes (fastest it can run), will fetch all commits with this and iterate over them, running git node land on each:
   a. If git node land finds an issue, Action adds a label manual landing required, adds a comment explaining what happened, and removes the commit queue label
   b. If it succeeds, push the changes and closes the issue with a comment pointing to the commit
3. For each PR landed on master, GitHub Actions CI will run, and if it fails, a revert PR will be opened, pinging the author, reviewers and collaborator who added commit queue. If this is a false negative, folks are free to close the PR.

This workflow has two potential issues: 1) if we use the GITHUB_TOKEN to commit those changes, our push CI won't be triggered. If, on the other hand, we use a personal token (we can use the github-bot user for that), it will trigger, but GitHub warns against it as we could run out of resources (although this is only a problem if we land significantly more PRs with the commit queue, otherwise we'll still run the same amount of jobs).

We'd also need to ensure the push job is not stopped when a new commit lands on master, otherwise we could end up with several commits landed and jobs are still running.

If no one objects on this approach I can get something together in the next few weeks (maybe sooner). We can improve later, but I think we need to roll out something.

Also, I know we have https://github.com/nodejs/node-auto-test, but maybe we should have a separate repository to try this out?

[lundibundi]

My other concern would be the spam of revert PRs in case there is a particular incompatibility between a few of them (i.e we land 4 PRs, 2nd changes something that due to changes in 1st PR is now broken and the bot will then open 3 reverts because each CI after 2nd now fails) or which is way more common CI failures due to flaky tests.
Could we instead ping relevant people in the PRs that have their master-ci failed and add separate comment-driven action to create a revert PR as a convenience tool and allow people to just manually revert/fix that at the same time? As much as more-PRs is not really a concern it adds up to the existing PRs and clutters them as much as notifications of people even further (we have 353 PRs currently open) which may drive the attention\time away from other PRs.

[mhdawson]

@mmarchini I assume that commit_queue would only be added after the full CI (if needed for the PR) is green, right?

[mmarchini]

My other concern would be the spam of revert PRs in case there is a particular incompatibility between a few of them (i.e we land 4 PRs, 2nd changes something that due to changes in 1st PR is now broken and the bot will then open 3 reverts because each CI after 2nd now fails) or which is way more common CI failures due to flaky tests.

PRs are easy to close without merging in case of false-positives, and having PRs will caught the attention of more folks than just a couple of people involved in the original PR. At first I thought about opening a PR only for the first failing CI, but if two subsequent commits fail (I know it's unlikely, but it could happen), the second one would get shadowed.

If the notifications are a big concern though, we could just do nothing if the CI fails on master as it will be caught up by someone eventually running CI on a PR, but I worry this would end up confusing users trying to open a PR and having their tests fail for an unrelated reason (and they would probably waste time trying to figure out why it's happening). I'd rather we do something though.

Could we instead ping relevant people in the PRs that have their master-ci failed

We could. In terms of mention notifications, that would generate the same (or more) than opening a PR. The tradeoff between this and opening a PR is that I would expect the PR to fix master faster because...

and add separate comment-driven action to create a revert PR as a convenience tool and allow people to just manually revert/fix that at the same time

...the Actions running on our PRs don't have write permission, thus they can't create a revert as a reaction to a comment. We would need to implement it as another scheduled action (well, it could be the same, just running reverts before landing anything new) and would likely use a revert-queue label of sorts. Or we could create an issue when a commit fails on master (since Actions on issues have the appropriate permissions), but that's not different than opening a PR, it just adds an extra step.

Nothing in the proposal is set in stone, so we could go either way (create PR, create issue, comment on PR, do nothing, something else). This can be implemented last and we could try a few approaches. Or we could choose one for now but have the option to change it later. Either way I don't want this to block, and as I said, let's try to keep it simple so we can get something running, even if the solution is not perfect.

@mmarchini I assume that commit_queue would only be added after the full CI (if needed for the PR) is green, right?

Initially yes, but we could also make the Action check if the last full CI finished and was green/yellow, and skip the PR if the CI is still running (this way we wouldn't need to worry about going back to a PR, check if CI finished, and then add a label, which is quite the turnaround consindering our CI takes 40+ minutes). Note that for the approach described in my last comment to work, the Action has to run all the checks we already have in place with node-core-utils, and will only land if all those checks pass (which is also how --non-interactive works on node-core-utils, which is the flag we'll need to use for the action).

[mmarchini]

Ok, I got the ultra basic workflow working (just land, reverting on failures still tbd)! First PR merged with a commit queue as proposed above on a test repository: mmarchini-oss/automated-merge-test#12. Also, it leaves a comment if git node land failed (pinging author and committer still tbd, although maybe it is not needed): mmarchini-oss/automated-merge-test#8 (comment)

This was a lot more tricky than I expected: ncu requires a Jenkins token to read if CI was successful or not, and for some reason the GITHUB_TOKEN doesn't work on ncu when reading some pull requests via the GraphQL API (not all, I assume it's related to who's the PR author). Funny enough, GITHUB_TOKEN works just fine on the scheduler event via the REST API with curl (including write actions), even to the PRs where GraphQL API calls failed with FORBIDDEN. I had to create a personal GH token to circumvent the GraphQL issue (we could create one for github-bot if needed).

I'll document how everything works in the README of the repository above once I get a change, just wanted to share because I'm happy this is finally working ^_^. For anyone curious, most of the logic is in land-stuff.sh.

[mmarchini]

Ok, I've been thinking more about how to revert when things fail. One option would be to revert in the push workflows, but this has many problems: GitHub doesn't let users declare dependencies across workflows, so either we would need to unify all files which run on push into one, or we would need to duplicate the revert logic on every workflow. Furthermore, GitHub hard stops a job when a step fails, to run a "cleanup step" we would need to add a lot of workarounds to every workflow, regardless of them being in the same file or not.

An alternative would be to use another scheduled action, which will look for the last X commits on master and will propose revert for the first one that fails if all subsequent also fails. The upside is that this can reduce noise, but as mentioned before, it could potentially shadow failures on other commits. We could also revert every red commit which doesn't have a revert PR yet.

So I'm starting to lean towards manual reverts using the same workflow proposed for landing: collaborators add a 'revert-queue' label to the PR, and the scheduled action does the rest. We might need a git node revert for this to work properly though.

[mmarchini]

Ugh, just hit another wall and honestly I'm starting to get tired of GitHub Actions 😅

So according to the docs, events triggered by GITHUB_TOKEN will not trigger Action runs. I tested it and confirmed that's the case. The docs also say that if you use a personal token then the Action will run. I tried using a personal token to push but the push event is still not triggered, which means the CI is not running for commits landing on master. I'm not sure if this is a bug on GitHub or if I'm still using GITHUB_TOKEN without knowing. Any help is appreciated, the code is in this repo and I described the implementation in the README.

[mmarchini]

Ok, never mind, this was an implicit behavior of the checkout action. I needed to pass a personal token to the checkout action too. Will probably start a PR on core tomorrow to discuss adding this. In the first iteration I suggest we leave for collaborators to manually revert: using the commit queue shouldn't be so different from git node land that it would cause more failures on master. In fact, there's not much difference on behavior between the commit queue and a collaborator who decides to bulk land a few PRs at once (the risk of failing on master is the same). Implementing an easier way to revert PRs could be done in parallel to the commit queue initiative.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.