Procedure for rolling out node-accept-pull-request

[orangemocha]

nodejs/node is finally in place and we are almost ready to move v0.x branches from joyent/node to it. However we still using two different workflows for landing pull requests, with node-accept-pull-request for v0.x and with a more manual procedure for everything else.

I have been working on getting node-accept-pull-request available for all branches, and it will be ready for broader consumption within the next week or so. Ideally, I'd like to roll it out on a trial basis, letting collaborators try it with some flexibility for using the old procedure. However, because of the way that it is designed, it kind of requires everybody to follow the same procedure. Here's why:

1. It rebases the PR onto the target branch (adding PR-URL and Reviewed-By to all the commits)
2. It builds and test the changes (like node-test-pull-request)
3. If all non-flaky tests pass, it does a fast-forward merge and pushes the updated branch to GitHub.

So if someone were to push manually to the same branch while a run is in progress between steps (1) and (3), the branch update at (3) would fail. This is somewhat desirable, because we want it to test the result of the rebase, and push exactly what it has tested.

Given the above, I think the best way for trying it out would be for all collaborators to start adopting it at the same time. Of course, if after some time we are all miserable because of it, we can revisit the process and even go back to the manual procedure. Before the roll-out, I will write a wiki page to document how to use it. There is also a procedure for landing other people's changes, to address a concern raised by @trevnorris.

I just wanted to give the TSC a heads up and a chance to discuss this plan. Adding to tsc-agenda.

@nodejs/tsc

[thefourtheye]

I love the idea of automating the PR landing process. Two questions,

1. when exactly we can start trying the new job?
2. we sometimes have a FIXES entry also in the commit log and that is missing in the job configuration. Does that mean, the committer has to explicitly add that in the commit log?

[bnoordhuis]

How does it know which Reviewed-By tags to add? Does it scan the PR comments for LGTM comments from contributors? If so, I suppose there's some kind of whitelist involved?

Aside: I love the idea of never having to manually merge a PR again.

[thefourtheye]

@bnoordhuis I see four Reviewed by fields in the Job configuration. So, whoever is landing will fill those fields I guess.

[bnoordhuis]

Guess I should have clicked on the actual link, eh? :-)

[bnoordhuis]

@orangemocha Can you set the repo to nodejs? It still points to joyent.

I'm guessing it's using an old toolchain? Pretty much all the linux bots are failing with:

cc1plus: error: unrecognised debug output level ‘line-tables-only’

The -gline-tables-only in common.gypi is not a recent change, it was added in ff7c68c from October last year.

[orangemocha]

Correct, reviewers are added manually, though scanning for LGTM is certainly doable and we can add that improvement after launch.

I'll add FIXES to the form. Thanks for the feedback!

[orangemocha]

@bnoordhuis sure, I'll update the repo to nodejs before launch. Like I said it's almost ready for use in node/js node, but not quite ready yet.

I'm guessing it's using an old toolchain?

Nope, it's using node-test-commit internally. Could those failures be related to #2376 ?

[orangemocha]

when exactly we can start trying the new job?

Pending work items:

• Raspberry Pis need an updated git
• Landing this PR
• Marking current flaky tests as such
• Ideally I'd like to make the ARM build parallel to speed the job up. If @rvagg has time to set up the new Raspberry Pis. Could launch without it.
• Add Fixes: to the form
• Update default inputs to nodejs/node
• Write wiki page with instructions.

More or less I'd say in a week from today :)

[thefourtheye]

@orangemocha I would say, you can make that a checklist and tick the items when they are done ;-)

More or less I'd say in a week from today :)

Good plan!!! 👍

[mscdex]

I'd think scanning for LGTMs would be tricky, since many times we will add conditions in the same comment (e.g. "LGTM if CI is happy"). Unless we start specially formatting our LGTMs or removing them until the condition has been met, to prevent early acceptance.

Also, what about the possibility of making the Reviewed-By form fields a dropdown or combo box?

[orangemocha]

Also, what about the possibility of making the Reviewed-By form fields a dropdown or combo box?

Sounds like a great idea.