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.

[trevnorris]

Workflow question. Are contributors expected to LGTM a patch before it's been through CI, or will CI have to run twice?

[orangemocha]

No harm in running CI twice, the first time to give feedback (as we do today with node-test-pull-request) and a second time to land it (with node-accept-pull-request). Obviously a PR needs to pass review before it can be landed.

Having the final CI run makes the first pass not a strict requirement. I guess it depends on the preferred workflow of the reviewers and on the level of confidence that the specific change will not break anything.

[trevnorris]

@orangemocha Cool. Just curious. :)

[orangemocha]

I am populating a huge dropdown for PR reviewers, with the names and email addresses of collaborators from https://github.com/nodejs/node#tsc-technical-steering-committee / https://github.com/nodejs/node#collaborators.

Having the github username there would make it easier to find and select reviewers. It also seems sensible to add the github username to the commit message. It would look like:

Reviewed-By: @github_username - FirstName LastName <email_address>

Does that sound like a reasonable change?

[thefourtheye]

Since we select github username from drop down it makes sense to include it. But the length of the lines will be much more than 72 :'(

[jasnell]

Sounds reasonable, yes, but @thefourtheye does make a good point. Don't think it's avoidable tho

[orangemocha]

We already make exception to the 72 column rule for URLs. IMO that rule makes sense for the commit message body but is less important for the metadata.

[jbergstroem]

@orangemocha what about using the same naming convention as in README.md?

[Fishrock123]

It also seems sensible to add the github username to the commit message.

Please no. I already get enough notifications. I don't need more from commits being ported all around.

[orangemocha]

@jbergstroem the premise is that it would be easier to find by username, and so we would put that first and sort alphabetically.

@Fishrock123 : that's a very good point 👍 Testing if that really happens... did you get a notification for orangemocha/node@4d4de8e ?

[Fishrock123]

Yes I got a notification. I've had it be pretty annoying before.

[orangemocha]

Ok, agreed that's a no-go. What if we removed the @ symbol?

Reviewed-By: github_username - FirstName LastName <email_address>

[orangemocha]

... which would be consistent with #2322

[rvagg]

+1, alternatively Reviewed-By: (github_username) FirstName LastName <email_address>

[thefourtheye]

I am okay with including the github username (without @ is also fine), because most of the times I can't associate actual names to the GH usernames and I know most of you guys by GH username only 😆

[orangemocha]

The ultimate bikeshedding tool : http://doodle.com/nf9tdwcxi7n6bfu6

[Trott]

I voted for all the options, which is about the same as not voting at all. Except now I get to feel like MY VOICE HAS BEEN HEARD!!!!!

[orangemocha]

Thanks for sharing your opinion on the poll. Option 1 won by a tight margin, so I am implementing that. The poll is now closed.

Reviewed-By: github_username - FirstName LastName <email_address>

[orangemocha]

All changes are in place and the workflow has been documented in the wiki. The target time for switching is this coming Monday. I made the announcement in a separate issue (#2598), so I am closing this one now.